DevCamp presentation: PR to merge or not to merge

From Dolibarr ERP CRM Wiki
Jump to navigation Jump to search

Cette page est issue de la présentation réalisée le 7 juin par Laurent Destailleur lors du Devcamp Dolibarr de Montpellier.

Version initiale rédigée par Lionel Vessiler

Nous allons commencer par mettre quelques options Github pour une meilleure visualisation des différences de fichiers et ensuite nous verrons quelques cas exemples pour lesquels on étudiera si on merge ou pas une PR.

Options Github

Pour gagner du temps et avoir une meilleure visualisation des différences, on peut commencer par paramétrer Github pour enlever les espaces et ne pas avoir trop de lignes différentes entre l'ancienne et la nouvelle version du fichier modifié.

Cacher les espaces

  • cliquer sur la roue dentée bleue
  • on peut aussi utiliser le mode "Split" pour une meilleure lisibilité entre le fichier avant et après les modifications
  • puis cliquer sur le bouton pour "appliquer et recharger" les options

Quelques exemples

Cas 1

Si on a une erreur avec croix rouge au niveau du résumé de la CTI, alors on ne merge pas. L'auteur est invité à consulter le détail pour corriger sa PR.

Fix-merge.png

Cas 2

https://github.com/Dolibarr/dolibarr/pull/26813/commits/b6cee0fcd5c05f9e29380b8fc01ddf8d5149e89d

Usecase2.png
  • L'usage d'un variable globale (qui normalement doit être évité) est acceptable pour un fix sur une branche de maintenance car il permet une correction du bug "à moindre impact". Mais sur la branche « develop », l'ajout de variable globale n'est pas toléré car on priorise la qualité de code long terme. La solution est donc le compromis suivant:
    • ici le mergeur va accepter la PR sur la branche ancienne pour obtenir la stabilisation de la vieille version en maintenance, mais va ajouter une ligne de commentaire incluant "@TODO" ou "@FIXME" dans le code, ou créer une Issue pour rappeler qu'il faudra revoir ce code pour la branche "develop".
  • le développeur est invité à faire une version différente sur la branche "develop" pour ne pas utiliser la variable globale.

Cas 3

Si on voit un début de transaction "$db->begin" dans une boucle "for" alors on ne merge pas

  • sauf cas très particulier, il faudra sortir les transactions "begin" de la boucle ainsi que les "commit" et "rollback"

Cas 4

https://github.com/Dolibarr/dolibarr/pull/27643/commits/afda2429682d6bcb9e75089de98fb4c9b23c0092

FIX SQL error

  • ici on se pose la question :
    • est-ce qu'on peut corriger une requête SQL juste en rajoutant un champ ?
  • dans ce cas, on évite les "DISTINCT" et on essaie de réécrire la requête SQL
  • le problème provenait du cumul d’un "DISTINCT" avec un "ORDER BY" qui n’était plus accepté par MySQL

Cas 5

Si on n’arrive pas à reproduire le bug alors on ne merge pas

  • c'est le cas lorsque la base de données est corrompue

=> on ne merge pas et au final c'est une PR pour ajouter un outil de réparation de la base de données (dans le /install/repair.php par exemple) plus qu'il vaut mieux faire (si toutefois, on peut reproduire, il faudra aussi corriger ce qui crée la corruption de donnée). Enfin on peut aussi imaginer une PR pour signaler l'état corrompu des données non en phase avec le métier, quand cela arrive, pour inviter à lancer l'outil correctif.

Cas 6

On a ici un déplacement d’un paquet de code

  • le trigger doit être dans la méthode Update
  • problème de paramètre passé dans la page pour appeler le trigger qui n’a pas été repris correctement

=> régression et on ne merge pas

    • pour corriger, il faut ajouter $notrigger = 1 dans la page pour garder le même comportement qu'auparavant

Cas 7

https://github.com/Dolibarr/dolibarr/pull/25819

Règles sur les fonctionnalités dépréciées :

  1. mettre @deprecated dans les commentaires du code de la fonction à  ne plus utiliser (sera supprimée dans plusieurs versions)
  2. mettre un syslog pour mettre en déprécié lors de l'utilisation de la fonction (visible dans les logs de Dolibarr)
  3. mettre les informations dans le fichier « Changelog »
Usecase7.png

=> on accepte la PR dans le branche "develop" uniquement. Remarque: Si la PR avait consisté à casser la compatibilité, la PR peut être accepté après un délai avec une couche de compatibilité, mais uniquement si elle contient aussi une mention dansle fichier ChangeLog pour annoncer la rupture de compatibilité.

Cas 8

https://github.com/Dolibarr/dolibarr/pull/26128

Usecase8.png

Ici on a un risque de non compatibilité

=> dans ce cas et si ce n'est pas trop long on peut utiliser l'éditeur en ligne (Edit file) depuis Github

Cas 9

https://github.com/Dolibarr/dolibarr/pull/24481

  • refactoring pour éviter « params » qui est utilisé dans les listes
  • on a un doute et l'on demande au développeur
  • c’est un problème d’initialisation dans une boucle
  • rien ne permet de dire que c’est une régression et pas de problème visible dans le code

=> après plusieurs échanges avec le développeur, au final, c'est corrigeable en une ligne de code

https://github.com/Dolibarr/dolibarr/commit/465fe0ffd4129030bd28c65249e86971ef014e80

Cas 10

https://github.com/Dolibarr/dolibarr/pull/22506/commits/b60629ea6f2e02958327a6ffd864be8416f7c4cf

Usecase10.png
  • attention : on remplace ici un "continue" (donc du code après qui n'était jamais exécuté) par du code qui sera systématiquement exécuté. Hors, la PR est sensé corrigé un défaut qui se manifest dans une certaines condition. On devrait donc s'attendre à un changement de code qui conditionne le nouveau comportement sur quelquechose. Comme ce n'est pas le cas, cela doit soulever des questions auprès du mergeur qui doit dans ce cas remonter son interrogation plutôt que valider. La PR semblant corriger un bug en en ajoutant un autre. Ceci peut se faire via des échanges avec l'auteur, en allant jusqu'à réussir à reproduire le problème lui même pour réorienter l'auteur sur la bonne solution si nécessaire.
  • finalement ici, il s'agissait d'un problème de « checkbox » qui n’est pas coché qui se comporte comme si il n'y avait pas de checkbox du tout (fonctionnement de base des chackbox en HTML)
    • il fallait mettre une condition en mettant un champ caché sur le formulaire avec une valeur fixe par exemple pour permettre au code serveur de faire la distinction entre checkox non coché (on modifie la base avec la valeur non), ou pas du tout de checkbox soumis dans le formulaire (on ne fait rien).

=> ceci est un cas de bug qui avait déjà été corrigé en introduisant un autre bug (bug yoyo qui corrige la cas A en cassant le cas B puis corrige le cas B en cassant le cas A...) et il fallait le corriger différemment depuis le champ "checkbox" du formulaire ainsi :

https://github.com/Dolibarr/dolibarr/commit/ee2ed963e8e97981028b76ab06a7c653d0e990b3

Cas 11

https://github.com/Dolibarr/dolibarr/pull/27050/commits/e8ddba34fabbdb8ea12088ad1e064f9c9d2c38da

  • New trigger CRONJOB_ERROR
  • ici on a une mauvaise utilisation des triggers et il aurait fallu utiliser des hooks. Voir [Système_de_Hooks] et [Système_de_Triggers].
  • le choix du trigger aurait été valable si on avait des transactions avec un test de retour pour savoir si on "commit" ou "rollback" les données et si l'action était de type CRUD (Create/Read/Update/Delete) d'un objet métier.

=> on ne merge pas et il faudrait proposer une nouvelle PR avec l'utilisation de hooks

Cas 12

https://github.com/Dolibarr/dolibarr/pull/29749/

  • ici on a un paquet de lignes dont le rôle est difficile à déterminer à vue d'oeil.

=> au final il faut plus de commentaires de la part du développeur

Et pour les évolutions

En ce qui concerne les évolutions dans Dolibarr, il n'y a pas de règles précises mais plusieurs éléments à prendre en compte :

  • respecter l'ergonomie de Dolibarr
  • se questionner sur
    • qui en a besoin ?
    • est-ce une nouvelle règle fiscale qui doit-être mis en place rapidement ?
  • posséder une meilleure évolutivité (vision à très long terme) et ne pas juste répondre à un besoin d'un projet client

A priori on devrait passer par le système d'Issues et solliciter le mergeur dès que l'on a une vision sur ce qu'on veut implémenter et comment on compte le faire, pour en discuter en amont de la fonctionnalité à apporter dans le projet Dolibarr.

Retrieved from "https://wiki.dolibarr.org/index.php?title=DevCamp_presentation:_PR_to_merge_or_not_to_merge&oldid=61154"