Coding style

[bwesterb]

Laat Python code voldoen aan een paar niet-onredelijke PEP8 eisen: (E701,W291,E301,E302,E231,E713,F401,E225,E261,E20*,W602). Welke willen we nog meer?

[bwesterb]

@aykevl ok?

[aykevl]

Ik ben nogal voorstander van automatische formatting. Ik ben het gewend van Go. Het maakt code veel leesbaarder.

Wederom niet doorgelezen (diff is veel te groot!) maar als het werkt prima. Liever wel met script er bij, zodat je voor een commit dat script nog kunt runnen om de code in de juiste stijl te krijgen. (Of zit het script er al in?) Ik heb geen specifieke voorkeur voor een bepaalde stijl, als het maar consistent is met wat gebruikelijk is bij Python. En als dit systeem maar in één stijl is.

[mrngm]

Even het aantal toegevoegde lege regels geteld, want de eerste paar diffs hadden alleen maar dat:

$ git diff origin/master HEAD | grep -E '^\+$' | wc -l
646

PEP8 schrijft ze voor, maar specifiek deze whitespace-toevoegingen zou ik los mergen van alle nuttige stijl-aanpassingen. De diff wordt er té groot van, zoals @aykevl al opmerkt, waardoor het reviewen onoverzichtelijk wordt.

Daarentegen, haalt flake8 wel wat trailing whitespace weg, wat me dan weer wél een plezier doet:

$ git diff --check HEAD origin/master | grep trailing | wc -l
21

Verder zie ik in $ git diff origin/master HEAD | grep noqa dat je nogal wat F401-errors (unused imports) onderdrukt, namelijk 73. Bijvoorbeeld in utils/shell.py zeg je from kn.leden import giedo, maar een korte zoektocht naar voorkomens van giedo blijkt dat dit alleen voorkomt in docstrings. Waarom de fout onderdrukken en niet de dead import daadwerkelijk weghalen?

Nu ik toch kijk voelt het een beetje krom om in c05e279 remove dead imports 46x # noqa: F401 toe te voegen en de dead imports niet te verwijderen. Zo'n commit zou alleen deletions moeten hebben.

Splits zoiets dan op in een commit met daadwerkelijke verwijdering van dead imports en eentje waarin je expliciet dead imports negeert. Maakt wederom het reviewen makkelijker.

[bwesterb]

@aykevl Ik heb autopep8 -aar . --select $code met $code bijvoorbeeld E261 gebruikt. Dat werkt niet perfect: flake8 --select $code gaf dan soms nog wat waarschuwingen, die ik met de hand heb gedaan.

Ik ben van plan er meer toe te voegen, maar wil bij elke wijziging controleren of er niets raars gebeurd – vandaar dat ik niet simpelweg de veranderingen van autopep -aar . commit. Ik kan een .travis.yml toevoegen die automatisch flake8 uitvoert, maar dat is misschien te negatief. Misschien zouden we een pre-commit hook kunnen maken die automatisch autopept? Ik ga er wat mee spelen.

[bwesterb]

Fijn dat je er kritisch naar wilt kijken :).

Even het aantal toegevoegde lege regels geteld, want de eerste paar diffs hadden alleen maar dat:

$ git diff origin/master HEAD | grep -E '^+$' | wc -l 646 PEP8 schrijft ze voor, maar specifiek deze whitespace-toevoegingen zou ik los mergen van alle nuttige stijl-aanpassingen. De diff wordt er té groot van, zoals @aykevl https://github.com/aykevl al opmerkt, waardoor het reviewen onoverzichtelijk wordt.

Klopt, vandaar had ik aparte commits gemaakt. Ik zal voor je gemak bij de volgende wijzingen aparte PR’s maken (hoewel die dan wel snel gereviewd moeten worden want zulke stijlveranderingen door elkaar mergen wordt waarschijnlijk iffy.) Daarentegen, haalt flake8 wel wat trailing whitespace weg, wat me dan weer wél een plezier doet:

$ git diff --check HEAD origin/master | grep trailing | wc -l 21 Verder zie ik in $ git diff origin/master HEAD | grep noqa dat je nogal wat F401-errors (unused imports) onderdrukt, namelijk 73. Bijvoorbeeld in utils/shell.py https://github.com/karpenoktem/kninfra/blob/c05e279607d48194b55754d6804066f7e60286cc/utils/shell.py#L18 zeg je from kn.leden import giedo, maar een korte zoektocht naar voorkomens van giedo blijkt dat dit alleen voorkomt in docstrings. Waarom de fout onderdrukken en niet de dead import daadwerkelijk weghalen?

utils/shell.py wordt ingeladen in de ipython shell als je “shell” uitvoert (zie $REPO/bin). Als je in de shell zit wil je soms direct spelen met giedo, vandaar dat die geïmporteerd wordt.

De andere dead imports zijn voor _import.py die gebruikt wordt om de Django omgeving op te zetten voor de scriptjes die vanuit de commandline worden gestart.

Nu ik toch kijk voelt het een beetje krom om in c05e279 https://github.com/karpenoktem/kninfra/commit/c05e279607d48194b55754d6804066f7e60286cc remove dead imports 46x # noqa: F401 toe te voegen en de dead imports niet te verwijderen. Zo'n commit zou alleen deletions moeten hebben.

Splits zoiets dan op in een commit met daadwerkelijke verwijdering van dead imports en eentje waarin je expliciet dead imports negeert. Maakt wederom het reviewen makkelijker.

Zal ik wat netter doen.