Style de code de Kresus

Kresus utilise un style de code défini principalement par des vérifications ESLint :

• https://framagit.org/bnjbvr/kresus/blob/master/.eslintrc.yml
• https://framagit.org/bnjbvr/kresus/blob/master/client/.eslintrc.yml

Une mise à jour d’ESLint entraîne des changements très peu esthétiques, selon l’avis de plusieurs contributeurs :

• https://framagit.org/bnjbvr/kresus/merge_requests/474

Comme ce style n’était pas parfait, il a été proposé d’utiliser Prettier à la place, pour ne plus avoir à se soucier du style du tout. Une merge request a même été ouverte :

• https://framagit.org/bnjbvr/kresus/merge_requests/482

ESLint a été choisi à la base pour sa flexibilité (choix des règles, etc.), et trouve un double usage : des vérifications statiques de code quand c’est possible (qui permettent d’éviter des erreurs par la suite) ainsi que des vérifications stylistiques (espaces ou tabs, etc.).

Il est ainsi proposé ici d’effectuer un changement qui convienne à tou.te.s les contributeur.ices en code. Les options possibles sont les suivantes :

• continuer à utiliser ESLint en acceptant la nouvelle règle d’indentation.
• continuer à utiliser ESLint en désactivant la règle d’indentation.
• utiliser prettier et effectuer la réécriture automatique du code avec prettier (et désactiver les règles stylistiques d’ESLint qui posent problème).

Si vous avez d’autres propositions, nous pouvons également les considérer.

Afin d’atteindre un consensus, je propose que chacun.e des personnes impliquées exprime son ordre de préférence parmi les propositions précédentes. Pour atteindre un consensus facilement, il est proposé de voter “pour”, “neutre” (= je ne suis pas pour mais je peux vivre avec) ou “contre”.

Personnellement, je pense qu’utiliser prettier est la moins mauvaise des trois solutions exprimées ici, et donne un code généralement agréable à lire. Je suis donc contre l’utilisation d’ESLint pour les règles de style, et suis neutre (plutôt pour) quant à l’utilisation de prettier.

@Phyks, @ZeHiro, @nicofrand, vos opinions ?

Cheers,
Ben

Je vote pour l’utilisation de Prettier.

Plusieurs options y ont été ajoutées depuis la première fois qu’on en a parlé, notamment l’indentation et la largeur des lignes, qui sont importantes à mes yeux.

Globalement je trouve le résultat plutôt pas mal. Il existe bien sûr quelques cas divergeant de mon goût mais le rapport concessions sur ces styles/temps gagné à chaque MR (que ce soit à l’écriture du code, au “make test” ou à la review) me paraît bénéfique.

Même chose que @nicofrand tout ce qui simplifie le process doit est bon à prendre. Les dernières config de prettier me plaisent bien.

Je vote pour prettier aussi, qui en plus permettra de régler les derniers styles qui ne sont pas forcés par la CI, telle que les fonctions avec des arguments sur plusieurs lignes ou que faire des accolades pour un if sur une seule ligne.

Il y a moyen de le faire tourner automatiquement par la CI Gitlab ? (je pense, mais je sais pas trop)

Ça serait top si c’était possible (un truc comme le lint sur chaque commit, qui rebase en passant prettier sur chaque commit <3). Sinon, à défaut, on pourra toujours le faire tourner pour vérifier qu’il a bien été passé avant de pusher.

Cool, j’ai l’impression qu’on arrive à un point d’accord ! Vu qu’on est tous pour, on peut passer le code vers prettier à un moment. J’ai déjà une MR qui fait ça, je la mettrai à jour une fois le moment venu, en désactivant les règles ESLint et en rajoutant l’intégration en CI (je pense qu’on pourra juste ajouter une vérification que le code est linté, mais pas le lint automatique, qui doit être un commit hook côté client)

Pour éviter que ce soit pénible pour les MRs en cours, je propose d’attendre deux choses avant de passer tout le code à prettier :

• qu’il n’y ait pas de “grosse MR” en cours, pour éviter 50 000 rebases super compliqués aux auteurs de ces grosses MRs. (chaque auteur de MR choisit lui-même ce qui est une grosse MR ou non)
• que la version 0.12 soit sortie, afin de faire une transition sur du nouveau code.

Si vous voyez d’autres choses à attendre, ou d’autres moyens de faire qui soient moins perturbants pour les MRs, je vous écoute

Cheers,
Benjamin

Rien à ajouter. Je considère !421 comme une grosse MR, parmi les miennes. Le reste c’est pas grand chose

Question subsidiaire : y’a quoi dans la 0.12, ou plutôt on release quand ?

Dans mes MRs,

• !434 est un WIP, avec des idées, qui va prendre un peu de temps à finir. Donc on s’en fiche un peu.
• !433 est “grosse” mais se finira assez vite dès que je m’y remettrai. Je pense qu’il ne faudrait pas l’inclure dans la version à venir par contre, trop de risques de péter un truc.

+1 pour lâcher une nouvelle version prochainement Comme ça on peut faire dégager Cozy et passer en Node v6 sur la nightly

Cool ! J’ai créé un label “before-prettier”, je vous invite à l’affecter sur les merge requests que vous aimeriez voir fusionnées avant que l’on migre vers Prettier. (Je vous invite à jouer le jeu et ne pas en rajouter de plus en plus au fil du temps )

Une fois qu’elles sont toutes fusionnées, je remettrai à jour ma MR et les règles de validation d’ESlint.

Cheers,
Ben