Das deutsche Python-Forum

Hallo zusammen,

da ich ab und zu in die Verlegenheit komme mal eben einen Ordner oder ein paar Dateien auf einem USB-Stick oder USB-Festplatte zu kopieren/synchronisieren, wollte ich ein einfaches Batch-Script.

Zu finden unter >> https://github.com/fillflag/fifosync/bl ... ifosync.py <<

Funktionen:
- Kann per Batch aufgerufen werden
- Versionierung (speichert ältere Versionsstände in einer ZIP-Datei)
- Cleanup (synchronisiert 1 zu 1 (Versionierung aus!) und löscht ältere Stände)

Für Kritik und Verbesserungsvorschläge bin ich immer offen

Viel Spass!

Ohne die genaue Funktionalität nachvollzogen zu haben:

- fehlende Dokumentation. Was genau macht das Tool? Speziell bei einem komplexen CLI Programm solltest Du da investieren und einen Module-DocString einbauen. Ggf. sogar mit Aufrufbeispielen, wie man es aus man-Pages kennt.

- Code auf Modulebene vermeiden!

- Nutze Stringformatierungsmethoden anstelle von "+". Also z.B. `"".format()`.

- Bei `copydir` würde ich mir mal `os.walk` angucken. Das erspart Dir ggf. auch die Rekursion.

- Nutze gute Namen für Deine Bezeichner. `x` und `y` sind das sicher nicht! Wieso nicht `name` anstelle von `x` ind `copydir`?

- vermeide globale Variablen! `cnt` ist da ein gutes Beispiel. Man wundert sich in der Funktion `copydir` was das für ein Objekt ist. Dann findet man es viel weiter unten als globale Variable. Neben der umständlichen Zuordnung bergen globale Variablen stets die Gefahr, dass diese ungewollte manipuliert werden während des Programmablaufs. Es gibt noch viele Gründe dagegen, hier stehen einige Gründe. (`global` verwendest Du zwar nicht, aber thematisch ist das egal)

- Dieser Name und das Objekt dahinter sind auch kritisch:

Code: Alles auswählen

#list [copy,versions/override, equal, delete]
cnt = [0,0,0,0]

Du brauchst also selber einen Kommentar, um zu erklären, was es mit dem Objekt auf sich hat? Wieso verwendest Du kein Dictionary, wenn Du doch "sprechende" Namen für die Einträge der Liste kennst?

Code: Alles auswählen

cnt = {"copy": 0, "versions":0, ...}

Und was bedeutet `cnt`?

- bei den imports würde ich auch zeilenweise vorgehen:

Code: Alles auswählen

# so nicht
import foo, bar, ...
# besser so
import foo
import bar
...

- Last but not least: Laut PEP8 sollte man 4 Spaces als Einrückung verwenden, nicht 2.

PEP 8 empfiehlt separate Import-Anweisungen für jedes Modul, und vier Leerzeichen Einrückung.

Zum Quelltext:

Lass den modulglobalen Quelltext, insbesondere das Parsen der Kommandozeilenargument, in Funktionen verschwinden. In seiner jetzigen Form kann man das Skript beispielsweise nicht im interaktiven Interpreter testen.
Benutze "argparse" statt "optparse".
Verwende keine abgekürzten Variablennamen, vor allem nicht auf Modulebene. Betrifft "p", "cnt", "x", "y" und möglicherweise noch andere Namen.
19ff: Die aufeinanderfolgenden "p.set_defaults()" lassen sich zu einer Zeile zusammen fassen, "p.set_defaults()" akzeptiert mehrere Argumente auf einmal.
26: "sys.exit()" statt "raise SystemExit()"
29, 33: Niemals "raise Exception()", immer konkrete Ausnahmetypen verwenden und im Zweifelsfall eigene Ausnahmetypen definieren. In diesem Fall kannst Du allerdings auch "sys.exit()" verwenden, de facto führt "raise Exception()" an diesem Stellen schließlich auch zum Beenden des Interpreters. Bei "sys.exit()" sieht der Benutzer allerdings wenigstens keinen an dieser Stelle völlig überflüssigen Traceback.
28ff: Die Existenz des Quellverzeichnisses zu prüfen, nur um im Falle des Fehlens wieder eine Ausnahme auszulösen, ist sinnlos und überflüssig. Beim Versuch, ein nicht existierendes Verzeichnis zu kopieren, wird ohnehin "OSError" ausgelöst.
37: Um Pfade zu vergleichen, musst Du vorher "os.path.normpath()" und "os.path.normcase()" anwenden, um die Pfade in eine kanonische Darstellung umzuwandeln. Andernfalls werden identische Pfade nicht erkannt, wenn sie über verschiedene symbolische Verknüpfungen referenziert werden (Unix) oder unterschiedliche Groß- und Kleinschreibung haben (Windows).
29,38,usw.: Benutze Zeichenketten-Formatierung mit dem "%"-Operator oder noch besser mit der ".format()"-Methode anstatt Zeichenketten über Konkatenation zusammen zu basteln. Das ist lesbarer und flexibler.
55, 60: "logging.info()" unterstützt Zeichenkettenformatierung, also auch hier keine Wortpuzzle mit Konkatenation.
Funktionen sollten niemals globale Objekte verändern. "copydir()" sollte die Anzahl der veränderten und gelöschten Dateien zurückgeben statt magischerweise ein globales Objekt zu aktualisieren.
Ebenso sollten Funktionen nach Möglichkeit nicht von globalen Objekten abhängen. Also kein Zugriff auf "cleanup" und "versions" in "copydir()". "versions" sollte als Parameter an "copydir()" übergeben werden, der Teil unter "cleanup", also 74ff, sollte sogar in eine eigene Funktion verschoben werden.
85ff: Missbrauche Zahlen nicht als Wahrheitswerte. Benutze die eingebauten Konstanten "True" und "False".

Zusammenfassend muss der gesamte Quelltext in Funktionen verschoben werden, die jeweils eine, und auch wirklich nur eine einzige Aufgabe haben. Also eine Funktion zum Parsen der Argumente, eine zum Kopieren einer einzelnen Datei, eine zum Kopieren eines Verzeichnisses und eine weitere zum Aufräumen. Je nach Bedarf natürlich noch weitere Funktionen. Die ganze globalen Variablen wie "cnt", "versions" und "cleanup" sind zu entfernen, die Funktionen müssen stattdessen über Rückgabewerte und Parameter miteinander kommunizieren.

Eine Anmerkung noch zur Verarbeitung der Kommandozeilenargumente. Optionen, also alles, was mit "-" beginnt, sind normalerweise optional (daher auch der Name Option). Auf "--src" und "--dest" trifft das allerdings nicht zu. Also sollten das auch keine Optionen, sondern vielmehr richtige Argumente sind. Kommandozeilenoptionen gehören auch nicht abgekürzt. Zuguterletzt sind Optionen, die "yes" und "no" entgegen nehmen, zu kompliziert. Stattdessen kann man einfach auf das Vorhandensein einer Option testen. Statt "%prog --src=<SourcePath> --dst=<BackupPath> --ver=yes --cln=no" sollte die Kommandozeile also besser so aussehen: "%prog [--version] [--cleanup] source destination".

Erstmal vielen Dank für die ganzen hilfreichen Hinweise und die Codeanalyse. Bin bereits dran das Script zu überarbeiten.

so habe das überarbeitete Script mal auf github gepostet ... hoffe die Liste wird jetzt ein wenig kleiner

https://github.com/fillflag/fifosync/bl ... ifosync.py

ty & hf

Das `raise` beim Beenden des Programms ist überflüssig, außerdem sollten Seiteneffekte möglichst nur im Hauptprogramm stattfinden, nicht in anderen Funktionen.
`build_optional_parameter` macht für mich keinen Sinn, warum machst du das nicht direkt in der `parse_commandline`-Funktion? Ist ja eigentlich nur das `if`. Dafür brauchst du ja kein Dictionary.
Laut PEP 8 gehören nach den Funktionsdefinitionen (ich nenn’ sie mal so) keine Leerzeilen hin.
Warum benutzt du in `copy_file` nicht os.path.is_file und os.path.is_dir? Falls nämlich `source` gar nicht existiert, wird angenommen, dass es ein Ordner ist.
Warum hast du dein Programm nicht richtig getestet? Da sind zum Teil logische Fehler wegen Tippfehlern drin. Bei den Kommentaren hast du dir anscheinend auch nicht viel Mühe gegeben, da ist in fast jedem ein Fehler drin.
Warum fängst du einen Fehler beim Kopieren einer Datei ab, ohne ihn zu behandeln?! (except IOError: pass)
Für os.path.split(foo)[-1] gibt es os.path.basename()

Update und versucht die vorhandenen Fehler auszumerzen

(Siehe Anfang)

Das deutsche Python-Forum

kleines Backup Script

kleines Backup Script

Re: kleines Backup Script

Re: kleines Backup Script

Re: kleines Backup Script

Re: kleines Backup Script

Re: kleines Backup Script

Re: kleines Backup Script