Coding Standards

Dev
#1

Salut,

Vous connaissez certainement les pages suivantes :

J’ai adopté le fichier de configuration pour PHP_CodeSniffer qui y est proposé il y a 2 ans quand j’ai démarré le développement du plugin des versions maintenue : phpcs.xml.dist

Je contribue très occasionnellement au plugin univers_spip et je me suis dit que se serait intéressant d’y appliquer les mêmes règles de codage. je les avais d’ailleurs appliquées pour spip_loader il y a longtemps sans toutefois versionner le fichier xml…

Par contre, quand on s’en sert pour SPIP lui-même, le rapport n’est pas fameux… :

--------------------------------------------------------------------------------------------------------------
A TOTAL OF 6090 ERRORS AND 733 WARNINGS WERE FOUND IN 298 FILES
--------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX 5901 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------

En affinant un peu (en excluant d’autres règles « PSR », qu’on suit peu ou pas en fait), on arrive à un résultat plus intéressant :

------------------------------------------------------------------------------------------------------------
A TOTAL OF 5821 ERRORS AND 6 WARNINGS WERE FOUND IN 207 FILES
------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX 5815 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------

L’idée de copier/coller un fichier dans différents dépôts git ne me satisfaisant pas particulièrement, j’ai créé un plugin pour PHPCodeSniffer, installable via composer et qui introduit aussi la vérification de compatibilité PHP.

Le fichier de configuration à utiliser pour SPIP ou un plugin quelconque en est simplifié : phpcs.xml.dist · master · James / supported-versions · GitLab

Ce plugin propose à ce jour 2 « rulesets » :

  • SCS1 pour SPIP Coding Standards 1 contient les règles définies de longues dates plus les quelques exclusions que j’ai ajouté,
  • SPIP40 qui inclut SCS1 et vérifie la compatibilité PHP pour SPIP 4.0, à savoir PHP 7.3-8.0

Je vais faire une PR pour les répertoires ./ecrire et ./prive pour montrer les changements…

Analyse statique du code
#2
#3

Tout ce qui peut amener au plus proche de PSR (de PHP-fig) me semble une bonne chose :slight_smile:

1 « J'aime »
#4

On pourra faire évoluer, et versionner les « SCS » progressivement :wink:

Je bosse (très lentement) sur le dev de « sniffs » spécifiques, comme par exemple le snake_case pour les variables et les noms de fonctions …

#5

Hello,

J’utilise php_cs_fixer sous PHPStorm avec un fichier de configuration que j’ai créé à partir du configurateur online PHP-CS-Fixer Configurator.

Je crois qu’on est plusieurs à avoir repris ce fichier et à l’appliquer (b_b par exemple).

#6

Oui j’ai joué avec ça pour tester, mais je ne l’utilise pas régulièrement. Amha, quel que soit l’outil et le format du fichier de conf, l’essentiel est d’avoir une conf dont le contenu est validé par la communauté des personnes qui contribuent au code et aussi que ça soit en ligne quelque part de reconnu et partagé, donc gogogo @JamesRezo !

PS : je partage mon fichier de conf spip.php_cs ici spip pastebin - outil de debug collaboratif - Bonjour les écureuils !

1 « J'aime »
#7

php-cs-fixer, effectivement c’est cool en ligne de commande, mais en terme d’extensibilité, c’est pas fameux. Voire inexistant. Et la distribution de règles custom n’est pas prévue nativement.

On peut aussi utiliser PHP_CodeSniffer dans VSCode et PHPStorm, il existe des extensions pour ça, et même des addons pour prettier…

#8

Un deuxième commit dans la PR ci-dessus, où on se base sur PSR-12 et non PSR-2, plus une règle sur la syntaxe des tableaux: $a = array(...) devient $a = [];

--------------------------------------------------------------------------------------------------------------
A TOTAL OF 4175 ERRORS AND 6 WARNINGS WERE FOUND IN 272 FILES
--------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX 4170 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------

Maintenant, comme l’a dit @b_b, mettons-nous autour d’une table pour décider quelles sont les règles, quelles est la base de départ (PSR-2 ou PSR-12 ?), quelles sont les exceptions à cette base, etc.

On devrait profiter de cette discussion pour réduire le nombre de pages web où ces règles sont décrites et la mettre à jour pour qu’il n’y ait plus d’ambiguité.

Je suis dispo jusque mi-septembre pour aider si besoin.

#9

À noter, et à mon sens c’est aussi important sinon plus, le rapport des erreurs signalés par phpcs non-autofixables : https://git.spip.net/attachments/862ee1c7-6c24-4167-9d8a-ea07e71f0fb0

#10

J’ai mis au point les règles spécifique à SPIP pourr le snake_case en prenant en compte les cas particuliers des fonctions spéciales de type balise_TRUC_dist, ça marche aussi pour les critères, les itérateurs et les boucles.

ça ne change pas les autofix, mais le rapport d’erreurs en dit long sur le snake_case :smiley:

--------------------------------------------------------------------------------------------------
A TOTAL OF 1160 ERRORS AND 6 WARNINGS WERE FOUND IN 73 FILES
--------------------------------------------------------------------------------------------------

Dont

------------------------------------------------------------------------------------------------
A TOTAL OF 151 ERRORS AND 0 WARNINGS WERE FOUND IN 26 FILES
------------------------------------------------------------------------------------------------

pour les fonctions

et

--------------------------------------------------------------------------------------------------
A TOTAL OF 1004 ERRORS AND 0 WARNINGS WERE FOUND IN 52 FILES
--------------------------------------------------------------------------------------------------

Pour les variables…

Alors, on s’y met autour de cette table (fusse-t-elle virtuelle ?) :wink:

#11

oui gogogo. Moi j’ai pas de religion précise sur les règles, comme b_b je pense que l’important c’est qu’elle soit partagées et publiques et qu’on ait un outil cli qui permette de fixer les erreurs de formatage (ou au moins de les signaler pour correction manuelle si besoin).

#12

On parle de quoi là ? Des erreurs de quoi ? On pourrait être plus précis ?

#13

Je parle de règles de codage et de l’outillage qui permet de détecter les écarts à ces règles dans le code.

Depuis décembre 2015, les règles de codage « officielles », sur le site spip.net, mentionnent PSR-2 et la première règle explicitée est :

Les variables, les fonctions et leurs arguments devraient être déclarées en snake_case

Je viens de programmer cette règle dans un plugin pour phpcs, facile à distribuer, à installer et à mettre à jour par des développeurs et de fournir le résultat de l’analyse qui montre les écarts entre le code de la branche master (fichiers php des répertoires ecrire/ et prive/ ) et les règles actuelles, dont la règle ci-dessus. Le plugin phpcs et la méthode de distribution peuvent aussi s’appliquer à d’autres dépôts git, pour peu qu’il y ait du code php à analyser.

La PR montre ce qui peut être corrigé automatiquement. Je n’ai pas programmé de « fixer » automatique pour le snake_case.

Au vu de cette analyse, je constate assez d’écarts pour estimer que ces règles sont peu ou pas suivies, que la documentation à ce sujet mériterait d’être mise à jour et mieux centralisées, et qu’il y a peut-être une stratégie à mettre en place pour résorber certains écarts non-fixables automatiquement, décréter que d’autres écarts sont sans importances, bref, qu’avant d’écrire quelque part dans spip.net : « Utilisez ce plugin de telle ou telle manière, voici ce qu’il fait, etc », il est encore temps de le peaufiner.

#14

Voici un exemple :

FILE: .../ecrire/inc/filtres.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 36 ERRORS AFFECTING 33 LINES
-----------------------------------------------------------------------------------------------------------------------------------------
  246 | ERROR | Variable "currentHead" is not in valid snake case format (Suggested name: current_head)
      |       | (SCS1.NamingConventions.SnakeCaseVariableName.NotSnakeCase)
  247 | ERROR | Variable "currentHead" is not in valid snake case format (Suggested name: current_head)
      |       | (SCS1.NamingConventions.SnakeCaseVariableName.NotSnakeCase)
  250 | ERROR | Variable "currentHead" is not in valid snake case format (Suggested name: current_head)
      |       | (SCS1.NamingConventions.SnakeCaseVariableName.NotSnakeCase)
etc.
-----------------------------------------------------------------------------------------------------------------------------------------

Ici, dans la fonction decrire_version_git(), la variable local $currentHead devrait être déclarée $current_head si on respectait l’écriture snake_case des variables.

Le plugin dit que c’est une « erreur », on pourrait décider qu’en fait c’est un warning et baisser par défaut la « sévérité » de cette erreur pour que l’outil ne le mentionne pas, par défaut, par exemple.

#15

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é.

#16

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.
#17

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

#18

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…

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