Auto-fix the code formatting

[magopian]

Fixes #1057

This PR is based on #1056 and shouldn't be merged before it.

This is highly debatable. And I know that creating a pull request modifying the code formatting is highly frowned upon.

So, I'm sorry. Also, don't hesitate to simply close this PR if you feel it's not a good idea!

Side note: the formatting is only checked on py3 because:
1/ black is only available for py3
2/ black can also reformat py2 code
3/ it's the same codebase, so no need to re-check in both builds (py2 AND py3)

[fpagnoux]

I don't think I spend enough time on the France codebase for my opinion to be relevant.

The E251 rule (level=logging.DEBUG without space) always made my fragile eyes suffer, but I think I'm alone against the whole Python community there.

Should be up to @guillett @benjello @claireleroy @frtomas (and others regular contributors) to discuss what good practice they want to adopt for this repo 🙂

(Aussi, pour information, le language par défaut des discussions sur OpenFisca-France est le français 🇫🇷 ⭐️ ⭐️ 😉 )

[benjello]

Je préfère vraiment que l'on garde des espaces autour des signes = même dans les appels de fonction.

[magopian]

mon opinion :
1/ quelles que soient les règles adoptées, il faut qu'elles soient vérifiées par un outil, parce que sinon il y aura toujours une violation qui passera au travers
2/ cet outil devrait aussi vérifier les règles de manière automatique

• par le CI : mise en échec lors d'une violation
• par une intégration aux éditeurs de code les plus courants : par une notification de violation
• par un pre-commit hook, pour éviter de commiter une violation par oubli ou mégarde
  3/ cet outil serait idéalement exécuté sur chaque sauvegarde d'un fichier : l'avantage est que le formatage devient un non-problème. On code, on sauvegarde, ça se reformate, on ne s'en soucie plus. Ce sont les méthodes préconisées par gofmt, rustfmt, prettier, elm-format etc... par le biais d'une intégration à l'éditeur. De cette manière un nouveau contributeur n'a pas besoin d'apprendre le formatage spécifique à ce repository, un outil le fait automatiquement pour lui
  4/ les règles adoptées devraient être la référence pour une communauté donnée. Pour reprendre les exemples ci-dessus, la configuration pour ces auto-formatteurs est minime (voire nulle) afin de supprimer toute différence entre différentes bases de code. J'ai l'impression que black est en train de devenir la référence pour la communauté Python, comme prettier l'est devenu pour JS, elm-format pour elm, rustfmt pour rust...

Pour l'outil black, ce qui me paraît pratique c'est qu'il fait le formatage automatique, et que sa configuration est minime (et je serai pour qu'il n'y en ai pas, pour garder la config par défaut).

[magopian]

J'ai ouvert une issue sur le dépôt de openfisca-code: openfisca/openfisca-core#706

Je pense qu'il est pertinent que cette discussion continue là-bas, afin d'impliquer un peu plus de monde dans la prise de décision, et d'adopter la même décision sur les deux dépôts.

[guillett]

Les contraintes des bases de codes varient d'un projet à un autre. Il me parait pertinent de rappeler le contexte spécifique d'OpenFisca-France.

Le code d'OpenFisca-France a vocation à être lu par beaucoup de monde : développeurs.ses bien sûr mais aussi économistes, juristes, journalistes et finalement toutes personnes intéressées par le système socio-fiscal français.

Je suis favorable à prendre des libertés et à s'écarter des conventions de formatage Python si cela facilite la lecture du code.

Les espaces autour des = est un bon exemple, cela rend le code plus aéré et, il me semble, plus accessible (ou en tous cas moins repoussant).

Cela étant dit, il me parait important de remettre en place un linter et la nécessité d'une configuration spécifique à OpenFisca-France me parait justifié.

My 2cts.

[magopian]

Je suis favorable à prendre des libertés et à s'écarter des conventions de formatage Python si cela facilite la lecture du code.

Je suis d'accord en théorie, et en même temps :

• un code plus aéré est aussi plus long, et plus "diffus" (a é r e r n ' e s t p a s f o r c é m e n t p l u s l i s i b l e)
• le coté repoussant/plaisant est très personnel

Par expérience il est très difficile d'arriver à un consensus sur ce genre de question, justement parce que les préférences personnelles, habitudes et expériences des uns et des autres prennent de l'importance, et peuvent cliver.

Donc la décision se prend en général soit :
1/ par un argument d'autorité : c'est le maintainer, le plus gros commiteur, le plus ancien commiteur, le chef de projet/d'équipe/le manager qui décide
2/ par un argument historique "on a toujours fait comme ça" : c'est le style utilisé jusqu'à présent qui doit être conservé (pas toujours facile quand le style actuel n'est pas cohérent dans tout le dépôt)
3/ par un argument "code is law" : tel ou tel outil est adopté par la communauté/répandu/pratique/"non-compromising" (sans configuration possible, donc sans discussion possible)

J'essaie de ne pas avoir d'avis personnel sur la mise en forme, et ne suis ni pour ni contre les espaces autour des =. Étant personnellement biaisé en faveur de la solution 3/ qui me semble retirer tout argument émotionnel/préférentiel de la discussion, il me semble qu'il est préférable d'avoir, par ordre d'importance

• un outil automatisé (je pense qu'on est d'accords sur ce point)
• un outil adopté (ou en cours d'adoption ?) par une grande partie de la communauté, pour que ceux qui lisent le code aient de grandes chances d'avoir déjà vu ou travaillé sur du code formaté de la même manière
• avec une configuration inexistante ou minimale pour que les autres projets qui ont adopté le même outil soient formatés de la même manière

[benjello]

Au nom de 1) et 2), j'aimerais que l'on garde les espaces autour des = est l'indentation plutôt que l'alignement lors de l'ouverture de parenthèses avec parenthèse fermante au niveaux des items.
Ce sont les seuls points sur lesquels je me vois argumenter longtemps ;-)

[magopian]

Il me semble que black ne peux être configuré pour accommoder ces demandes, donc si c'est la décision finale, il faudra trouver une autre solution, un autre outil.

Des idées @benjello ?

[benjello]

@magopian : désolé, je n'en vois pas mais je ne suis vraiment pas une référence.
Mais ceci dit, je suis vraiment pour que l'on ait un code automatiquement reformaté. Et, aux petits bémols formulés plus haut, j'appuie, pour ce que cela vaut, ton initiative.

[guillett]

@benjello, je comprends le problème de formatage pour les espaces mais pas pour "l'indentation plutôt que l'alignement lors de l'ouverture de parenthèses avec parenthèse fermante au niveaux des items" ; pourrais-tu filer un lien à une section du diff de cette PR qui met en évidence l'indentation que tu préfères versus l'indentation générée par black ?

De ça, il faudra regarder quelles règles de black désactiver pour être dans les clous.

Merci d'avance.

[benjello]

@guillett : je n'ai pas regardé. J'ai juste mentionné les points sur lesquels je suis prêt à argumenter longtemps et que j'ai souvent rencontré dans les formateurs des éditeurs par exemple. Donc si cela n'est pas apparu, go for it.

[magopian]

De ça, il faudra regarder quelles règles de black à désactiver pour être dans les clous.

Le concept, le principe de base de black est justement de ne pas donner de possibilité de configuration ("non-compromising"). Si j'ai bien compris https://black.readthedocs.io/en/stable/installation_and_usage.html#usage il n'y a que trois possibilités de modifier le formatage:
1/ la longueur de la line préférée (88 par défaut)
2/ le formatage uniquement compatible py36 qui consiste à avoir des "trailing commas in function signatures and calls" (désactivé par défaut)
3/ ne pas normaliser les (double) guillemets des strings

Ni gofmt ni elm-format ne permettent la moindre configuration, avec l'argument fort qui est "Il est utile pour une communauté d'avoir un seul et même style. Et il est donc souhaitable que chaque projet adopte le même style que celui de la communauté. Et donc si un projet veut bénéficier des avantages de l'outil de formattage, il devra adopter le même style que celui de la communauté". Il me semble même qu'en GO il n'est pas possible de ne pas adopter gofmt.

[magopian]

je suis prêt à argumenter longtemps

@benjello est-ce que ce sont des arguments basés sur tes préférences personnelles ? C'est totalement acceptable au nom de 1/ et 2/ comme indiqué plus haut, du moment qu'il est accepté que 1/ ou 2/ font loi sur ce dépôt !

Peut-être que autopep8 avec une configuration spécifique de pycodestyle pourrait arriver au résultat souhaité par @benjello, mais je ne le garantis pas.

Par ailleurs, il ne me semble pas que autopep8 soit largement utilisé dans la communauté, mais je peux me tromper, je n'en suis plus très proche depuis deux ans.

[guillett]

Une remarque supplémentaire : la communauté OpenFisca != la communauté Python

[magopian]

Malheureusement, il me semble que flake8 ne peut que vérifier, pas corriger (ce qui veut dire qu'en tant qu'humains on doit continuer à faire un travail de robot). La seule solution que je connaisse est autopep8 et pycodestyle que j'ai cités plus haut, mais je ne sais pas si ils permettent les espaces autour des = par exemple.

Si c'est la solution prise, je veux bien que quelqu'un prenne le relai pour investiguer ça, et je ferme cette PR.

[benjello]

@fpagnoux @guillett : voici ce que je sous-entend sur les parenthèses (crochets, etc) fermantes.

x = (
    a
    + b
    + c
    )

y = [
    riri,
    fifi,
    loulou
    ]

Sinon je vous laisse gérer les suites à données car je ne suis pas compétent (mais j'approuve même si je la ramène hein ;-) ).

[benjello]

J'ai promis une réponse à @magopian. La voici: je pense que ne pas avoir d'espaces autour des = dans les mots clés et les paramètres des fonctions (voir E251) est vraiment préjudiciable lorsque l'on a de longs noms de paramètres.
Or pour être explicite les noms sont assez longs dans openfisca généralement et je trouve que ne pas avoir d'espaces ajoute de la charge cognitive.

Mais je veux être constructif. Donc je renvois la balle ;-)
Est-ce que @guillett @Anna-Livia @fpagnoux @openfisca/core-contributors @openfisca/france-admin pourrait investiguer si l'on peut utiliser autopep8 ?

@magopian : j'ai lu la doc de black. Le but est d'avoir le moins de lignes de différences possibles mais le reformatage qu'il propose sont un peu loin de ce que l'économiste moyen intervenant sur openfisca ferait (et ce que j'estime lisible) donc même si je comprends l'intérêt pour une personne qui fait du python à longueur de journée, je pense que l'on se tromperait de cible.

[fpagnoux]

Je veux bien passer un peu de temps pour explorer les alternatives à black, dès que j'aurais terminé avec les chantiers déjà ouverts (sans doute semaine prochaine)

[bonjourmauko]

Au nom de 1) et 2), j'aimerais que l'on garde les espaces autour des = est l'indentation plutôt que l'alignement lors de l'ouverture de parenthèses avec parenthèse fermante au niveaux des items.
Ce sont les seuls points sur lesquels je me vois argumenter longtemps ;-)

@magopian D'après ce que j'ai cru comprendre, autopep8 accepte les mêmes options en config que flake8. Pour répondre à l'enjeu soulevé par @benjello, il me semble que le setup.cfg suivant en répondrait :

; E128 continuation line under-indented for visual indent
; E251 unexpected spaces around keyword / parameter equals
; F405 name may be undefined, or defined from star imports: module

[flake8]
hang-closing = true
ignore = E128,E251,F405
max-line-length = 120

[pep8]
hang-closing = true
ignore = E128,E251,F405
max-line-length = 120

[magopian]

Je peux me tromper, mais ces configurations permettent uniquement d'ignorer, pas de "auto fix".

[bonjourmauko]

En effet, ces configs auto-fixent le hang-close (parenthèse fermante au niveaux des items) mais pas les deux autres...

[bonjourmauko]

Après explorer les offres disponibles, en prenant en compte les retours de @benjello et @magopian, voici ma conclusion : ce n'est pas possible de renforcer le style désiré out-of-the-box. J'ai testé :

• black : uncompromising
• autopep8 : renforce seulement des PEP8s
• yapf : pourrait être tuné mais l'effort me semble assez fort

Ma proposition : suivre la solution autopep8, qui semble être la second best, et renforce +80% des cas, en attendant que de nouvelles outils soient disponibles ou qu'un·e contrib trouve une solution pour résoudre le last mile*.

* Quand je dis last mile, je veux dire que la solution autopep8 n'est pas exclusive par rapport au style désiré, mais tout simplement que, comme le soulève @magopian, ces cas seront tout simplement ignorés.

Qu'en pensez-vous ?

👍 👎

[fpagnoux]

Le sujet a aussi été discuté sur core: openfisca/openfisca-core#706

Je pense aussi qu'autopep8 serait un bon premier pas. Pour l'instant il n'y a aucun linting ni formatting sur France, et ça parait être une bonne première étape.

[benjello]

Toujours ok si mes lignes rouges sont respectées ;-)

[Morendil]

J'ai aussi testé Pylint. Bottom line:

------------------------------------
Your code has been rated at -9.18/10

(cf, Q6)

Le risque de bikeshedding me semble important.

Collectivement, comment évaluez-vous la valeur ajoutée qu'apporte le travail sur cette PR ? Et êtes-vous bien certain·e·s que le nombre de commentaires sur cette PR (proxy pour l'énergie investie dans la discussion) soit en rapport avec cette valeur ajoutée ?

En particulier, quand vous la comparez aux autres PR ouvertes sur ce dépôt qui ont reçu moins de 26 commentaires… C'est à dire, en fait, toutes les autres.

[bonjourmauko]

Collectivement, comment évaluez-vous la valeur ajoutée qu'apporte le travail sur cette PR ?

Personnellement je trouve que le manque de style explicite, donc de linting c'est en général un déterrent à la revue.

Et êtes-vous bien certain·e·s que le nombre de commentaires sur cette PR (proxy pour l'énergie investie dans la discussion) soit en rapport avec cette valeur ajoutée ?

Je crois que le fait qu'il ait autant nombre de commentaires c'est en réalité un proxy sur la valeur que cela a pour des différents contributeurs et contributrices. Tans les contributions, les revues, comme les commentaries étant faits pour une grande partie sui generis , bona fide et pro bono, sont à mon humble opinion de bons et de mouvais indices :

• D'une part, c'est un sujet généralisé, dont beaucoup s'emparent
• De l'autre, les PR avec moins de reviews et de commentaires méritent plus d'attention.

(Ce que ne nie pas la valeur de celle-ci).

Au-delà de ça, je partage la caution sur le bikeshedding, et ajoute donc ma proposition d'utiliser autopep8 comme second best, et pusse pour l'acceptation de #1114 😃.

[Morendil]

Remplacé par #1135