Coding Standards

Dev
#19

Une fois que les règles sont définies, plus besoin d’un configurateur :slight_smile: Et nos règles sont définies depuis 6 ans ^^, pas à jour, pas respectées, mais définies :wink:

et phpcs est tout aussi configurable avec vscode, phpstorm ou même sublime_text, mais le plus intéressant, c’est d’avoir un outil commun aux dev, via IDE ou en ligne de commande et via une CI automatisée (très facile à faire avec gitlab par exemple)

Je le redis, php-cs-fixer c’est pas extensible facilement, parce que pas conçu pour ça, c’est initialement prévu pour des application symfony avec un coding standard plus proche des PSR que nous. D’ailleurs, je dirais que nos règles étaient plus des « PSR-2 -- » que des « PSR-2 ++ » ^^

#20

OK. Je vais adapter le plugin en conséquence.

Pour ce qui est des lib importées, j’exclue le fichier https://git.spip.net/spip/spip/src/branch/master/ecrire/inc/idna_convert.class.php. Pour le code récupéré dans des librairies ou sur le net, évidement c’est plus compliqué. Je dirais que lorsqu’on décide de versionner du code externe DANS notre code, on devrait au minimum le rendre plus « spipien », non ?

Pour info, la PR de cette discussion s’appuie sur le fichier de config phpcs suivant :

<?xml version="1.0"?>
<ruleset>
    <file>./ecrire</file>
    <file>./prive</file>
    <exclude-pattern>ecrire/lang/*</exclude-pattern>
    <exclude-pattern>ecrire/inc/idna_convert.class.php</exclude-pattern>

    <rule ref="SPIP41" />

    <arg name="cache" value="tmp/.php_cs.cache"/>
    <arg name="report-full" value="tmp/php_cs.txt"/>
    <arg name="report-summary"/>
    <arg value="s"/>
</ruleset>
#21

Tututu, on est pas PSR2++, mais FIG-R cf GitHub - php-fig-rectified/fig-rectified-standards: The FIG PSR-2 that actually makes sense (not p ^^

#22

« FIG-R– » si tu veux :slight_smile:

#23

Avant j’aurais dit non pour des questions de maintenance, mais à partir du moment où on a des sepcs propres, partagées et automatisables, oui bien sûr !

#24

Le plugins spip/coding-standards est à jour.

Je vais pousser une autre PR. Elle contiendra le fichier de configuration par défaut ci-dessus, une mise à jour du fichier .gitignore ainsi qu’un fichier composer.json minimal. (la PR : #4865 - feat(spip/coding-standards): Mise en place de l'outil - spip - SPIP on GIT)

  • ATTENTION!: Il ne s’agit en aucun cas de l’introduction de composer en tant qu’outil de développement pour SPIP!
  • Son seul ojectif, pour le moment, est de permettre l’installation et la mise à jour d’outils de développement annexes et des règles de codage.
  • Je recommande de ne pas utiliser composer.json pour introduire des librairies comme ça trop vite pour SPIP lui-même. Discutons-en à tête reposée avant de se lancer là-dedans :wink:

Par la suite, il sera possible de (re)faire dans un second temps des PRs d’autofix, soit totales, soit partielles comme l’a suggéré @rastapopoulos, me semble-t-il. Je supprimerai la PR qui a servi de démo dans la foulée.

Pour utiliser PHP_CodeSniffer, il faudra :

  • installer composer Introduction - Composer
  • exécuter la commande composer install qui aura pour effet de télécharger phpcs, phpcbf (l’autofixer), et les règles SPIP ainsi que de compatibilité PHP.
  • exécuter la commande vendor/bin/phpcs aura pour effet de lancer une analyse, de générer un cache (tmp/php_cs.cache) qui sera exploité par les analyses suivantes, un rapport complet dans le fichier tmp/php_cs.txt et un résumé dans la sortie standard.

Pour changer le comportement de phpcs, il suffit de copier phpcs.xml.dist en phpcs.xml à la racine du site, puis d’en modifier le contenu. La documentation est plutôt complète.

Dernier conseil : il est bon de se familiariser avec l’exécution des outils de dev en ligne de commande avant de commencer à les intégrer dans nos IDE préférés, histoire de comprendre comment ils fonctionnent.

Bonus : sans rapport direct et ça intéressera @marcimat en particulier, j’introduis la configuration pour la fabrication du fichier zip destiné à files.spip.net via composer. Ex: composer archive --format=zip --dir=tmp --file=spip génère le fichier tmp/spip.zip

Si tout cela vous convient, je ferai des propositions de mise à jour de la documentation sur spipnet.

1 « J'aime »
[spip ↪ 4.0] feat(spip/coding-standards) : Mise en place de l'outil (...)
[spip] 2 commits
[spip ↪ cs-tool] feat(spip/coding-standards) : Mise en place de l'outil (...)
#25

Un truc en passant, je ne sais pas si vous jugerez utile de reporter ça dans la branche 4.0. Si tel est le cas, il ne faudra pas oublier de modifier le fichier phpcs.xml.dist la branche 4.0 pour utiliser le standard SPIP40 au lieu de SPIP41 afin de tester les bonnes versions PHP.

#26

On peut déployer des outils CI/CD

On peut aussi pour commencer à brancher le check sur ses git hook locaux. Cela évite de pousser du code mal formaté, indépendamment du reste.

#27

Il est fortement souhaitable de reporter aussi en branche 4 sinon les cherry-pick entre le code reformaté de la branche master et le code non reformaté de la branche 4 vont être pénibles…

1 « J'aime »
#28

^^

C’est noté, je ferai le report :slight_smile:

#29

Et je tilte on sur les projets synchronisés sur github, on utilise déjà scrutinizer
https://scrutinizer-ci.com/g/spip/SPIP/
et je crois qu’il supporte phpcs dans leurs fonctionnalités.

1 « J'aime »
#30

Je viens de tester à partir de la branche de ta PR, nickel ça fonctionne au top. Juste une question, peut-être HS, est-ce qu’on peut générer un rapport avec des stats par sniff/rule pour avoir une idée des « erreurs » les plus présentes ?

#31

Par par défaut.

Il y a des « metrics » relevés par certains « sniffs », mais pas tous. Et pour les restituer, il faudrait coder un rapport spécifique. Je me note ça pour le futur du plugin. Merci pour l’idée ! :slight_smile:

Sinon, la plupart du temps, ce sont les services d’intégration continue qui brassent des rapports au format standard (checkstyle) pour mouliner des jolis graphiques. Mais ça reste une bonne idée de penser à quelque chose pour la ligne de commande. :wink:

#32

Je me réponds, je viens d’appliquer phpcbf et le diff me semble très bien. Maintenant je me pose la question suivante : est-ce qu’on envoie toutes les modifs d’un coup dans un seul commit (ce qui permet de minimiser la "pollution de l’historique, et donc de faciliter les blame plus tard) ou est-ce qu’on envoie par paquets, genre un commit par sniff (au moins pour les array to [] et single quotes) ?

Amha la première option est la bonne, mais je me dis que la seconde permet d’y voir plus clair dans les diffs de chaque commit. Vos avis ?

#33

Le 26/08/2021 à 19:44, b_b via Discuter de SPIP a écrit :

Amha la première option est la bonne, mais je me dis que la seconde permet d’y voir plus clair dans les diffs de chaque commit. Vos avis ?

C’est un peu ce que je disais l’autre jour, car sur l’énorme diff c’était long de voir si on détectait des erreurs, faux positif, etc. Certaines modifs sont basiques et surtout beaucoup plus courantes : les quotes au moins, peut-être d’autres. Une fois qu’on a commité ça, les autres diffs sont beaucoup moins gros et donc on voit mieux ce qui change.

–
RastaPopoulos

#34

On se rejoint, je pense que ça vaudrait le coup de séparer les commits au moins pour les quotes et les array to [], mais de l’autre côté de la balance il y a le fait que ça génère plus de bruit dans l’historique :\

#35

Comme on utilise git, une solution serait de faire une branche, séparer les commits (ce qui permet de voir le détail) et une PR et à la fin on peut la squasher (ou la merger si on préfère)

#36

:champagne: c’est mergé et reporté en 4.0

Donc, la suite, c’est une branche à partir de master, un commit par transformation importante, une PR qui sera squashée

Je propose que pour la 4.0, on fasse l’opération après et en une seule fois

Pour les transformations, je regarde de manière un peu détaillée, il y a peut-être plus que les quotes et les short arrays …

1 « J'aime »
#37

La PR : https://git.spip.net/spip/spip/pulls/4868

les commits

#38

Donc, on merge ?