lunar hat geschrieben:Naja...
Ich gehe das einfach mal von oben nach unten durch
Schön das sich jemand die Mühe macht
lunar hat geschrieben:
Auch in Docstrings solltest du die von PEP 8 vorgeschlagene maximale Zeilenlänge von 80 Zeichen einhalten. Scrollbalken im Code-Block sind halt hässlich
Kommt davon wenn man vom Editor einfach den Text in den Browser kopiert
lunar hat geschrieben:
Grundsätzlich solltest du Konstanten mit Großbuchstaben bezeichnen, damit man sie im Code auch erkennt. Das betrifft die fünf Namen, die du auf Modulebene bindest.
Da hast du natürlich recht, aber die globalen Variablen wollte ich sowieso noch loswerden
lunar hat geschrieben:
Pfade setzt man mit os.path.join zusammen. ``os.getenv`` ist zwar nicht falsch, aber imho ist der Zugriff auf ``os.environ`` eher üblich.
Danke, guter Tipp, werde ich ins Zukunft benutzen
lunar hat geschrieben:
Für Logging gibt es das gleichnamige Modul in der Standardbibliothek, es besteht also kein Grund, Logging selbst zu implementieren.
Vor allem, weil dein Logging nicht sehr performant ist, da die Datei bei jedem Logeintrag neu geöffnet werden muss. Außerdem wird die Datei nicht richtig geschlossen, wenn in Zeile 42 eine Ausnahme auftritt. Das führt dazu, dass geöffnete Dateideskriptoren zurückbleiben. Dadurch besteht die theoretische Möglichkeit, dass der Kernel dein Skript killt, weil es bei langen Laufzeiten die Ressourcenlimits überschreitet.
Mag ja stimmen, aber bei einem Skript was alle 10min einen Befehl ausführt ist die Performance ja nicht unbedingt ein Hauptkriterium. Ich hab hier einfach den leichten Weg der Mikrooptimierung vorgezogen
lunar hat geschrieben:
In Zeile 44 verfällst du einer in letzter Zeit imho ziemlich häufig zu beobachtenden Unsitte: Ein except ohne Ausnahmetyp. Damit fängst du alles ab, auch Syntaxfehler, was sicherlich nicht in deinem Interesse ist. Wenn du IO-Ausnahmen behandeln willst, solltest du nur EnvironmentError abfangen. Ein einfaches except ist höchstens dann sinnvoll, wenn du den Traceback loggst, also beispielsweise ``logging.exception`` verwendest (ein weiterer Grund, dass ``logging`` zu verwenden). Generell solltest du ein ``except`` ohne Ausnahme aber meiden wie der Teufel das Weihwasser
Das gilt natürlich für jedes ``except`` in deinem Code.
Das ist wohl einfach nur Bequemlichkeit. Nicht schön, aber naja.....
Wieder so eine "Quick&Dirty"-Unsitte...
lunar hat geschrieben:
Ob es Aufgabe eines Dämons ist, die Beispielkonfiguration zu erzeugen, sei mal dahingestellt. Eigentlich liefert man die Beispiel-Konfiguration mit dem Programm aus, so dass der Nutzer sie auch vor dem ersten Programmstart zur Verfügung hat.
Tja, von den ersten 10 Leuten die mein Script anfangs benutzten waren 6 Stück leider nicht in der Lage, "echo $home/dir > .bgcd_config && echo 300 >> .bgcd_config " einzugeben. Natürlich ist das nicht die Aufgabe eines Daemons, aber es schadet ja auch nicht, oder?
lunar hat geschrieben:
Zeile 66 ist eher sinnlos. Damit öffnest du nur die Dateiobjekte neu, die Standardströme bleiben trotzdem mit dem Terminal verbunden, da die Dateideskriptoren 0, 1 und 2 nicht neu vergeben werden. Im Gegenteil, du erzeugst einfach drei neue Dateideskriptoren 3, 4 und 5, die auf /dev/null verweisen. Korrekterweise solltest du zuerst alle Dateideskriptoren schließen, um alle Verbindungen zur Umgebung des Vaterprozesses zu schließen. Danach solltest du 0, 1 und 2 mit ``os.open`` neu öffnen, um die Dateideskriptoren neu zu binden, so dass auch C-Code nicht mehr die alten Standardströme nutzen kann.
Im Cookbook gibt es ein
Rezept zum Forken eines Dämons, dass du dir ansehen solltest.
Habe ich getan und festgestellt, dass das wohl auch nicht der Weißheit letzter Schluss ist. Ist es wirklich nötig zweimal zu 'forken'? Andere Tutorials/HowTo's etc. kommen auch prima mit einem Fork aus.
lunar hat geschrieben:
Zum Parsen einer Konfigurationsdatei solltest du eines der verfügbaren Module wie ConfigParser oder ConfigObj nehmen, die gehen auch mit nicht existierenden Konfigurationsdateien korrekt um, da du Standardwerte zuweisen kannst. Dann sparst du dir die Krücke in Zeile 83 ff.
Außerdem schließen diese Module die Konfigurationsdatei wieder, so dass die benötigten Ressourcen wieder freigeben werden.
Danke, werde ich mir mal anschauen
Ich beschäftige mich erst seit einer Woche mit Python, da kenne ich bei weitem noch nicht alles. Deswegen finde ich es gut auf sowas hingewiesen zu werden
lunar hat geschrieben:
In Zeile 94 fehlt die die komplette Fehlerbehandlung. Wenn der Nutzer keine korrekte Fließkommazahl in der Konfiguration angibt, stürzt dein Programm stillschweigend ab. Wobei ich mir nicht mal sicher bin, ob der Traceback wirklich verloren geht. Immerhin ist Dateideskriptor 2, also der Standardfehlerstrom noch offen, wenn die Ausnahme also in C-Code abgefangen wird, könnte der Traceback sogar noch auf dem Terminal erscheinen.
Stimmt, habe das Python-Typecasting wohl überschätzt.
lunar hat geschrieben:
Zeile 101 und 102 kannst du dir komplett sparen, dass ist nur ein eher schlechter Workaround, zu dem du gezwungen bist, weil du os.path.join nicht verwendest. Gleiches gilt für die Stringkonkatenation in Zeile 109 und 112.
Ein Workaround ja, aber warum soll der schlecht sein?
Und warum os.path.join(path,dat) verwenden, wenn auch path+dat das gleiche Ergebniss liefert, wo ich doch genau weiß, dass path+dat einen validen Pfad ergibt?
lunar hat geschrieben:
In Zeile 112, 113 ist deine Einrückung imho kaputt.
Da kann man sich die fehlenden Tabulatoren einfach hindenken
lunar hat geschrieben:
Wenn du, wie in Zeile 116 überprüfen willst, ob eine Liste leer ist, nutze die Tatsache, dass leere Listen im bool'schen Kontext als ``False`` ausgewertet werden: Statt ``if len(dl) == 0:`` also ``if not dl:``.
Naja, das ist wohl eher Geschmackssache, genauso wie manche lieber i=i+1 statt i+=1 schreiben.
lunar hat geschrieben:
Du wirst jetzt sagen, dass du die Länge eh in Zeile 125 nochmal brauchst, aber statt einen zufälligen Index in der Liste auszuwählen, kannst du auch gleich direkt das Element "ziehen": ``random.choice`` ist das Mittel der Wahl
Guter Tipp, werd' ich mir merken
lunar hat geschrieben:
Bezüglich Zeile 133: Hast du das Programm schon mal mit Dateinamen, die Leerzeichen oder Anführungszeichen enthalten, ausprobiert? Du wirst feststellen, dass das nicht funktioniert. ``os.system`` nutzt die Shell zum Starten des Unterprozesses, ergo musst du korrekt quoten. Am besten nimmst du aber gleich das ``subprocess``-Modul.
Leerzeichen oder Anführungszeichen in Dateinamen?
Wer macht den sowas?
Solche Unsitten gibt's bei mir nicht.....
lunar hat geschrieben:
Zeile 156, 157 ist sehr merkwürdig. Warum nur machst du das so obskur? Du könntest ``usage`` auch gleich direkt aufrufen. Oder das Feature nutzen, dass ``sys.exit`` auch einen String als Argument akzeptiert.
Wollte ich auch schon längst geändert haben
lunar hat geschrieben:
Bezüglich Zeile 161: ``elif`` existiert.
Naja, bei 2 if-Zweigen kann man das doch verkraften
lunar hat geschrieben:
Wenn du schon in der stop-Funktion SIGTERM zum Töten des Prozesses nutzt, dann solltest du das auch richtig abfangen, um den Prozess sauber zu beenden.
Und wie sollte das dann genau aussehen?
Weiß nicht warum das nicht reichen sollte, aber ich lasse mich da gerne aufklären...
Danke für Deine Kommentare, werde mir einige Anregungen mal genauer Anschauen. Du weißt ja, wenn man nicht fragt, erfährt man nichts. Deshalb ist es wichtig seinen Mist mal irgendwo zu posten
Ach ja, hier mal mein ursprüngliches 3 Minuten Script, bevor ich daraus ein Daemon hacken wollte:
Code: Alles auswählen
import time
import os
import random
import sys
dl=[]
old=-1
path=sys.argv[1]
tts=float(sys.argv[2])
for dat in os.listdir(path):
if os.path.isfile(path+dat):
if dat.endswith(".jpg"):
dl.append(path+dat)
while True:
i=random.randrange(0, len(dl))
while i==old:
i=random.randrange(0, len(dl))
old=i
os.system("feh --bg-center " +dl[i])
time.sleep(tts)
Nicht weltklasse, aber schnell hergestellt und funktioniert