Code style #200

New issue

Closed

opened 2018-10-02 14:17:06 +02:00 by delobell · 6 comments

delobell commented

2018-10-02 14:17:06 +02:00

(Migrated from git.eleves.ens.fr)

Situation actuelle

imports : un from .. import .. par module + tout les objets derrière,
on surveille ce que dit "flake8" mais sans être contraignant,
on se limite à 80 colonnes.

Discussions

Imports

Les lignes d'imports sont généralement sources de conflits lors des merges.
Pour réduire ça, on pourrait utiliser la convention de n'importer qu'un seul objet par ligne.
Ça peut faire des préambules un peu long, mais :

si on rage trop, ça devrait nous encourager à "découper" les modules en plus petits modules,
les IDEs et éditeurs sont généralement capables de "réduire" ces lignes d'imports, ou de commencer
l'affichage après celles-ci.

On peut automatiser ça avec un plugin comme isort.
Il peut en plus classer les imports par type, améliorant la lisibilité.

Note : Django utilise isort.

Linter

Chacun est libre de formater le code à sa sauce : flake8 ne vérifie ~~que~~ la PEP8, qui autorise
de multiples formatages pour une même ligne de code.
Je vous propose d'utiliser à nouveau un plugin : black.

L'intérêt de ce plugin : rien n'est négociable et son formatage est déterministe (ou presque).
Ainsi, Michelle sur sa machine peut obtenir le même formatage d'un module que Michel sur une autre machine.
Cela réduit énormément le nombre de conflits, et ça permet de moins se poser de questions qui n'ont
pas un grand intérêt !

Pour cette raison, black est de plus en plus utilisé dans la communauté Python open-source.

L'intégration aux IDEs, éditeurs se passent généralement bien.
Seul point critique : il faut avoir python3.6 d'installer pour le faire tourner.

Nombre de colonnes

On voit l'intérêt des 80 colonnes : 2 fichiers ouverts sur un même (petit) écran, et c'est bien !
Mais c'est quand même très petit... Les auteurs de black ont l'air d'accord et propose, par défaut, d'utiliser
une largeur de 88 colonnes.
Avec ça on peut encore ouvrir deux fichiers côte-à-côte sur un 14" ☺

Propositions

black

Ajout de black à la CI en --check
Au préalable, l'appliquer à toute la codebase, se fait "rapidement" :
- un commit sur master avec l'ajout de la conf (fichiers à inclure, exclure),
- passer black sur master,
- cherry-pick le commit de conf sur chaque MR (et branche) et aussi appliquer black.
  → Et ça sans conflit supplémentaire !

isort

Ajout de isort en à la CI --check
Configurer isort en 1 import / ligne (on peut aussi garder le multi-imports par ligne, mais ça fait du conflit)

colonnes

Passage au nombre de colonnes proposé par black (88), qui semble être
un bon compromis pour du 14" en moyen/gros caractère avec 2 fichiers

à voir

On pourrait ajouter isort et black dans un hook de pre-commit, mais j'ai un peur que ça
soit frustrant pour les nouveaux arrivants de pas réussir à commit pour ces raisons.
(git commit -n ferait l'affaire)

Du tooling se met en place pour gérer la conf et l'exécution de ce type d'outils.
Si des solutions sont viables et très faciles à utiliser alors on pourrait les intégrer
au projet pour avoir du hook au pre-commit, etc.
Ça reste à étudier...

### Situation actuelle - imports : un `from .. import ..` par module + tout les objets derrière, - on surveille ce que dit "flake8" mais sans être contraignant, - on se limite à 80 colonnes. ### Discussions #### Imports Les lignes d'imports sont généralement sources de conflits lors des merges. Pour réduire ça, on pourrait utiliser la convention de n'importer qu'un seul objet par ligne. Ça peut faire des préambules un peu long, mais : - si on rage trop, ça devrait nous encourager à "découper" les modules en plus petits modules, - les IDEs et éditeurs sont généralement capables de "réduire" ces lignes d'imports, ou de commencer l'affichage après celles-ci. On peut automatiser ça avec un plugin comme [isort](https://github.com/timothycrosley/isort). Il peut en plus classer les imports par type, améliorant la lisibilité. Note : Django [utilise](https://github.com/django/django/blob/master/setup.cfg) isort. #### Linter Chacun est libre de formater le code à sa sauce : flake8 ne vérifie ~que~ la PEP8, qui autorise de multiples formatages pour une même ligne de code. Je vous propose d'utiliser à nouveau un plugin : [black](https://github.com/ambv/black). L'intérêt de ce plugin : rien n'est négociable et son formatage est déterministe (ou presque). Ainsi, Michelle sur sa machine peut obtenir le même formatage d'un module que Michel sur une autre machine. Cela réduit énormément le nombre de conflits, et ça permet de moins se poser de questions qui n'ont pas un grand intérêt ! Pour cette raison, black est de plus en plus utilisé dans la communauté Python open-source. L'intégration aux IDEs, éditeurs se passent généralement bien. Seul point critique : il faut avoir **python3.6** d'installer pour le faire tourner. #### Nombre de colonnes On voit l'intérêt des 80 colonnes : 2 fichiers ouverts sur un même (petit) écran, et c'est bien ! Mais c'est quand même très petit... Les auteurs de black ont l'air d'accord et propose, par défaut, d'utiliser une largeur de 88 colonnes. Avec ça on peut encore ouvrir deux fichiers côte-à-côte sur un 14" ☺ ### Propositions **black** - Ajout de black à la CI en --check - Au préalable, l'appliquer à toute la codebase, se fait "rapidement" : * un commit sur master avec l'ajout de la conf (fichiers à inclure, exclure), * passer black sur `master`, * cherry-pick le commit de conf sur chaque MR (et branche) et aussi appliquer black. → Et ça sans conflit supplémentaire ! **isort** - Ajout de isort en à la CI --check - Configurer isort en 1 import / ligne (on peut aussi garder le multi-imports par ligne, mais ça fait du conflit) **colonnes** - Passage au nombre de colonnes proposé par black (88), qui semble être un bon compromis pour du 14" en moyen/gros caractère avec 2 fichiers **à voir** On pourrait ajouter isort et black dans un hook de pre-commit, mais j'ai un peur que ça soit frustrant pour les nouveaux arrivants de pas réussir à commit pour ces raisons. (`git commit -n` ferait l'affaire) - Du tooling se met en place pour gérer la conf et l'exécution de ce type d'outils. Si des solutions sont viables et *très* faciles à utiliser alors on pourrait les intégrer au projet pour avoir du hook au pre-commit, etc. Ça reste à étudier...

lstephan commented

2018-10-03 16:29:16 +02:00

(Migrated from git.eleves.ens.fr)

Je suis plutôt favorable à black (sur 88c), ça évite de devoir corriger du PEP8 partout à chaque fois... Par contre pour isort, je vois l'apport pour les conflits mais je suis plus dubitatif sur le reste... J'ai l'impression que pas mal de conflits peuvent être réglés rien qu'en utilisant isort sans forcément mettre un import par ligne.

Je suis plutôt favorable à `black` (sur 88c), ça évite de devoir corriger du PEP8 partout à chaque fois... Par contre pour `isort`, je vois l'apport pour les conflits mais je suis plus dubitatif sur le reste... J'ai l'impression que pas mal de conflits peuvent être réglés rien qu'en utilisant `isort` sans forcément mettre un import par ligne.

delobell commented

2018-10-04 11:19:54 +02:00

(Migrated from git.eleves.ens.fr)

J'ai l'impression que pas mal de conflits peuvent être réglés rien qu'en utilisant isort

C'est vrai qu'on peut s'en contenter.

Pour info : https://github.com/timothycrosley/isort#multi-line-output-modes
Sur authens, j'avais dû mettre le 5.

> J'ai l'impression que pas mal de conflits peuvent être réglés rien qu'en utilisant `isort` C'est vrai qu'on peut s'en contenter. Pour info : https://github.com/timothycrosley/isort#multi-line-output-modes Sur authens, j'avais dû mettre le 5.

delobell commented

2018-10-06 14:41:07 +02:00

(Migrated from git.eleves.ens.fr)

Sur #200, j'ai finalement dû sortir la 3 pour la compatibilité avec black.

On aurait pu run black puis isort dans cet ordre, mais bon c'est facile à "obliger" dans un script. Plus compliqué si tu commences à les run à la main.

Sur #200, j'ai finalement dû [sortir](https://github.com/ambv/black#how-black-wraps-lines) la 3 pour la compatibilité avec `black`. On aurait pu run black puis isort dans cet ordre, mais bon c'est facile à "obliger" dans un script. Plus compliqué si tu commences à les run à la main.

delobell commented

2018-10-06 14:41:44 +02:00

(Migrated from git.eleves.ens.fr)

mentioned in merge request !317