Coding Standards

Ok, merci.

En fait, je me demandais d’où tu étais parti : de l’existant spip (PSR-2 ++) ou from scratch.
Sinon, pour le snake_case je pense que les cas où cela existe dans le core c’est : des librairies externes ou du code récupéré dans des librairies ou sur le net comme cela semble être le cas dans l’exemple que tu donnes.

Mon avis serait de corriger les cas du même acabit que ton exemple et de laisser les cas de librairies importées tels que. Donc il faudrait à terme ne remonter que des avertissements, une fois le premier jet de correction effectué.

Pour php-cs-fixer je trouve l’outil très bien foutu pour une utilisation avec PHPstorm par exemple. En plus, il y a un configurateur de règles super pratique qui permet de définir de façon précise ton ensemble de règles applicables.

J’avais utilisé cet outil d’ailleurs car je n’arrivait pas à faire fonctionenr à l’époque code_sniffer sous PHPStorm mais ce n’est pas une raison valable.

Par contre, j’aimerais aussi ajouter que ces règles et ces outils c’esy cool mais on devrait aussi avoir des règles qualité minimales comme:

  • Utiliser un éditeur qui vérifie la syntaxe et un peu plus et qui permet déjà d’éviter pas mal de bugs latents comme les variables indéfinies;
  • un run avec les notices pour les supprimer avant commit
  • des tests minimaux pour vérifier qu’un commit est ok ou ne balance pas une régression.

+1 pour ce cas précis, même si je ne vois pas trop la différence entre une erreur et un warning quand on parle d’analyse syntaxique de code, car après tout les gens pourront toujours passer outre tant que leur PR ne sera pas bloquée à cause de ça ^^

phpcs a un exit_code qui peut varier selon les paramètres qu’on passe à la CLI qu’on peut aussi configurer via le fichier xml par défaut : Configuration Options · squizlabs/PHP_CodeSniffer Wiki · GitHub

pour ce qui est du blocage de PR, on peut le faire avec des plates-formes git comme bitbucket ou github. Pour gitea, je ne sais pas…

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 ++ » ^^

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>

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 ^^

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

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 !

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

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.

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.

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

^^

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

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

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 ?

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:

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 ?

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

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 :\