betagouv / beta.gouv.fr

Le site public de l'Incubateur de Services Numériques de l'État
https://beta.gouv.fr
248 stars 1.01k forks source link

Uniformiser les espaces #390

Closed MattiSG closed 7 years ago

MattiSG commented 7 years ago
Morendil commented 7 years ago

C'est une bonne idée et je vois que tu as déjà commencé, mais ça reste en suspens. Est-ce qu'un coup de main là dessus te serait utile?

MattiSG commented 7 years ago

Oui : j'essaie (enfin, plus activement depuis quelques semaines…) de faire fonctionner https://github.com/jedmao/eclint sur la base du .editorconfig déjà en place. En fait, c'est un peu plus compliqué que prévu, et ça nécessite d'itérer sur la config elle-même. Si tu veux t'y coller, c'est avec plaisir.

Morendil commented 7 years ago

OK, j'ai utilisé la commande suivante pour faire tourner ECLint avec les exclusions qui me semblaient justifiées:

find . -type d \( -path ./_site -o -path ./.git -o -path ./bin -o -path ./rb -o -path ./img -o -path ./js/lib \) -prune -o -print | xargs eclint check

J'ai poussé sur la branche les quelques corrections de whitespace nécessaires à tout valider, sauf Gemfile.lock. (Corrections réalisées à la main avec Sublime Text.)

Perso ça m'est égal que Gemfile.lock soulève des erreurs mais ça peut gêner si tu veux ajouter ECLint à l'intégration continue ou à des tests auto en local, c'était ton intention?

Morendil commented 7 years ago

Hmm peut-être que l'exclusion de "rb" n'est pas justifiée.

Si je comprends bien quand tu dis "plus compliqué" et "itérer sur la config", le souci c'est qu'il faut ajuster les règles en fonction des sous-répertoires, types de fichiers (images etc), voire de la nature des fichiers concernés (les libs js minifiées n'ont pas besoin d'un newline à la fin).

Tu voudrais pouvoir faire eclint check **/* et laisser la config faire le tri?

Morendil commented 7 years ago

Avec quelques règles pour rb, js et Gemfile.lock j'ai réduit les exclusions à:

find . -type d \( -path ./_site -o -path ./.git -o -path ./img \) -prune -o -print | xargs eclint check

Morendil commented 7 years ago

Je m'interdis tout commentaire sur "la bonne" façon d'indenter et me déclare aussi agnostique à ce sujet que sur la question de rouler à droite ou à gauche, l'important c'est que localement tout le monde fasse pareil, et qu'on signale les changements aux frontières. :)

Mais il y a un souci potentiel avec ta config: elle prévoit d'indenter avec des tabs les fichiers textes usuels, dont les .md. Or le "frontmatter" des .md est du YAML, et je découvre avec effroi qu'on n'a pas le droit d'indenter du YAML comme on veut, les tabs sont interdits.

On aura donc une contrainte sur le frontmatter des .md, qui ne devra pas inclure d'objets composites; si c'est le cas on devra les mettre dans le config.yml. (A moins qu'il y ait d'autres solutions qui m'échappent pour le moment.)

MattiSG commented 7 years ago

s/ta config/la config actuelle/

Je me suis contenté de refléter ce qui était le plus fréquent dans la base de code actuelle. Je n'ai pas d'avis fort sur ce vers quoi aller et suis ok pour changer si cela simplifie la gestion des front-matters.

Je te confirme aussi que la complexité était bien dans la sélection des fichiers auxquels appliquer eclint, pas l'application en elle-même. J'ai essayé d'utiliser les globs pendant assez longtemps pour abandonner. Je suis ok pour utiliser find | xargs, mais cela implique de vérifier la portabilité de toutes les options…

Tu voudrais pouvoir faire eclint check */ et laisser la config faire le tri?

C'est ça. Mais la syntaxe des globs pour faire de l'exclusion est… complexe.