New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verbesserungen #1

Open
linusg opened this Issue Jan 12, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@linusg

linusg commented Jan 12, 2018

Hi Peter,

Besser spät als nie. Ich hoffe ich vergesse nichts. Bitte alles nur als gut gemeinte Tipps sehen, du musst natürlich nicht alles umsetzen. Ist auch Geschmackssache, teilweise.

python3-dbus, Version unbekannt

python3-dbus V? (MIT)

Bei den meisten Python-Packages bekommst du die Version als String unter package.__version__. Ist so eine Art inoffizieller Standard. In diesem Fall geht dbus.__version__, ich hab's getestet.

Eine ausführliche Anleitung befindet sich in ./latex/YAMuPlay.pdf

Beim Start von ./yamuplay.py im Terminalfenster erscheint im Terminal folgende Kurzanleitung:

Das würde ich zur besseren Lesbarkeit und als Klarstellung, dass es Pfade und Befehle sind so machen:

Eine ausführliche Anleitung befindet sich in latex/YAMuPlay.pdf.

Beim Start von ./yamuplay.py im Terminalfenster erscheint im Terminal folgende Kurzanleitung:

Oder auch noch verlinken:

Eine ausführliche Anleitung befindet sich in latex/YAMuPlay.pdf.

Beim Start von ./yamuplay.py im Terminalfenster erscheint im Terminal folgende Kurzanleitung:

Auch wenn ich die Situation eines Versions-Mismatch sehr gut kenne und dein Weg, die GitHub-Repositories in deines zu integrieren, ist das doch ein arg umständlicher und kontraproduktiver Weg.

Da es in der Vergangenheit immer wieder zu Kompatibilitätsproblemen kam, wenn die jeweils neueste Programmversion der verwendeten Python-Module heruntergeladen wurde, wird dem yamuplay-Repository ab V0.2.1 IMMER der tatsächlich verwendete Versionsstand der genannten GitHub-Repositorys der verwendeten Module hinzugefügt, um von inkompatiblen Updates und Änderungen durch die Maintainer dieser Module unabhängig zu bleiben.

Tipp: mache dich mal zum pip requirements Format schlau. Dann braucht es nur ein git clone deines Repositories und ein pip install -U -r requirements.txt!

in dieser Datei kannst du festlegen, was genau installiert werden soll: Jede Zeile ist im Format package??x.y.z, wobei package der Name des Pakets ist, ?? ein Vergleichsoperator wie == oder >= und x.y.z die Version. Gut beschrieben ist es hier: https://pip.readthedocs.io/en/1.1/requirements.html

Das würde alles in allem die Größe des git repositories reduzieren, andererseits die Installation vereinfachen. Und wenn du doch ein bestimmtes Paket upgraden willst, weil du ein neueres Feature brauchst und verifiziert hast, dass es läuft, reicht eine Änderung in der requirements.txt.

So, das war's zum Readme. Zum Code würde ich auch noch ein paar Worte sagen, als begeisterter Python Programmierer lege ich Wert auf sauberen Code und Styleguides/Best practices. Es gibt auch solche, die scheren sich kein bisschen darum, solange der Code tut was er soll. Solltest du zu dieser Gruppe gehören: nevermind und nicht weiter lesen 😉

https://github.com/schlizbaeda/yamuplay/blob/master/yamuplay.py#L88

In den Konstruktor kommen üblicherweise nur die Variablen, die man dem Konstruktor auch übergeben kann. PATH ist ein schlechter Name, weil Konstanten (in Python keine echte Konstanten) groß geschrieben werden, nicht aber Parameter. Da du eh Python 3 verwendest, brauchst du nicht mehr von object erben. Alle seit Python 3 gibt es eh nur noch new-style classes.

Statt so:

class YAMuPlay(object):
    def __init__(self, PATH = '/media/pi'):  # TODO: Unterscheidung wheezy (/media) und jessie (/media/pi)     
        # globale Variablen mit Vorbelegung:
        self.gl_appName = 'YAMuPlay'
        self.gl_appVer = '0.2.2'
        
        self.GL_PATHSEPARATOR = '/'          # TODO: os.path.sep() liefert das Pfad-Trennzeichen, unter LINUX '/'
        self.gl_MediaDir = PATH
        ...

also so:

GL_PATHSEPARATOR = '/'          # TODO: os.path.sep() liefert das Pfad-Trennzeichen, unter LINUX '/'


class YAMuPlay:
        # globale Variablen mit Vorbelegung:
        gl_appName = 'YAMuPlay'
        gl_appVer = '0.2.2'
        ...

    def __init__(self, path='/media/pi'):   # TODO: Unterscheidung wheezy (/media) und jessie (/media/pi)
        self.gl_MediaDir = path
        ...

Die Variablenbenennung ist auch nicht mit PEP8 im Einklang, aber das musst du mit dir selbst ausmachen. Lesenswert ist es allemal: https://www.python.org/dev/peps/pep-0008/

https://github.com/schlizbaeda/yamuplay/blob/master/yamuplay.py#L121

Sehe ich das richtig, dass du tatsächlich alle Argumente zu Fuß parst? Respekt...
Sowas macht man der Einfachheit halber üblicherweise mit argparse.

https://github.com/schlizbaeda/yamuplay/blob/master/yamuplay.py#L199

Wieder Benennung, wieder Geschmackssache. Aber per Konvention schreibt man Klassen CamelCase und Funktionen und Methoden mit_unter_strichen. ich war etwas verwirrt, weil ich eine Methode sah, die wie eine Klasse benannt ist. Das geht einem recht schnell so in den Kopf.

https://github.com/schlizbaeda/yamuplay/blob/master/yamuplay.py#L214

Oft ist es einfacher statt viele Strings mit + zusammen zu fügen, str.format zu verwenden:

geometry = str(w) + 'x' + str(h) + '+' + str(l) + '+' + str(t) # sowas im Format wie '1280x720+0+0'

vs

geometry = '{0}x{1}+{2}+{3}'.format(w, h, l, t)  # sowas im Format wie '1280x720+0+0'

https://github.com/schlizbaeda/yamuplay/blob/master/yamuplay.py#L351

Das Zen of Python sagt dazu:

Errors should never pass silently.
Unless explicitly silenced.

Bedeutet: kein nacktes except verwenden, das fängt auch die unerwarteten Fehler stillschweigend ab und kann zu üblen Bugs führen. Besser genau die eine Exception abfangen, die bei einer falschen Farbbezeichnung geworfen wird.

Ansonsten.... viel von dem bereits gesagten wiederholt sich oft. Immer wenn du viele str() und + brauchst ist ein str.format oft einfacher.

Du schreibst viele Kommentare (gut gut, ich kann mich nur selten dazu überwinden: Beispiel), aber in meinen Augen ist es hier zu viel des Guten. So wenig wie möglich, so viel wie nötig.

Und > 1400 Zeilen ist schon einiges, das könntest du vielleicht in mehrere Module aufteilen. Ist dann aber ein bisschen Arbeit. Überhaupt, den ganzen Code zu überarbeiten ist sicher ein gutes Stück Arbeit.
Tolles Projekt, hat ja schon dem einen oder anderen im Forum z.B. weiter geholfen!

So ich hoffe, du kannst damit was anfangen. Fragen gerne hier oder im Thread im Forum.

  • Linus
@schlizbaeda

This comment has been minimized.

Show comment
Hide comment
@schlizbaeda

schlizbaeda Jan 25, 2018

Owner

Hi Linus,

vielen Dank für Deine Hinweise, ich habe vor, die meisten demnächst nach und nach umzusetzen.
Die Datei README.md habe ich für den Commit für V0.3 mal angepasst.

Für die restlichen Punkte brauche ich etwas Zeit, geht nicht mehr im Fasching bis 13.02.2018 :-)

schlizbäda

Owner

schlizbaeda commented Jan 25, 2018

Hi Linus,

vielen Dank für Deine Hinweise, ich habe vor, die meisten demnächst nach und nach umzusetzen.
Die Datei README.md habe ich für den Commit für V0.3 mal angepasst.

Für die restlichen Punkte brauche ich etwas Zeit, geht nicht mehr im Fasching bis 13.02.2018 :-)

schlizbäda

@linusg

This comment has been minimized.

Show comment
Hide comment
@linusg

linusg Jan 25, 2018

Kein Problem, ich bin gespannt! 😃

linusg commented Jan 25, 2018

Kein Problem, ich bin gespannt! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment