30,909
edits
DevCamp presentation: PR to merge or not to merge: Difference between revisions
Jump to navigation
Jump to search
m
DevCamp presentation: PR to merge or not to merge (view source)
Revision as of 09:41, 10 June 2024
, 2 years ago→Et pour les évolutions
P.scoffoni (talk | contribs) (Version initiale) |
|||
| Line 5: | Line 5: | ||
Nous allons commencer par mettre quelques options Github pour une meilleure visualisation des différences de fichiers et ensuite nous verrons quelques cas pour savoir rapidement si on merge ou pas une PR. | Nous allons commencer par mettre quelques options Github pour une meilleure visualisation des différences de fichiers et ensuite nous verrons quelques cas pour savoir rapidement si on merge ou pas une PR. | ||
=== Options Github === | ===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é. | 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''' | '''Cacher les espaces''' | ||
* cliquer sur la roue dentée bleue | *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 | *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 | *puis cliquer sur le bouton pour "appliquer et recharger" les options | ||
=== Quelques exemples === | ===Quelques exemples=== | ||
'''Cas 1''' | '''Cas 1''' | ||
| Line 25: | Line 25: | ||
[[File:Usecase2.png|center|thumb]] | [[File:Usecase2.png|center|thumb]] | ||
* sur la branche « develop » on n’ajoute pas de variable globale et l'on priorise la qualité de code | *sur la branche « develop » on n’ajoute pas de variable globale et l'on priorise la qualité de code | ||
** ici le mergeur va saisir TODO ou Fix me ou créer une Issue pour rappeler qu'il faudra revoir le code pour la branche "develop" | **ici le mergeur va saisir TODO ou Fix me ou créer une Issue pour rappeler qu'il faudra revoir le code pour la branche "develop" | ||
* le développeur aurait du faire une version différente sur la develop pour ne pas utiliser la variable globale | *le développeur aurait du faire une version différente sur la develop pour ne pas utiliser la variable globale | ||
'''Cas 3''' | '''Cas 3''' | ||
| Line 33: | Line 33: | ||
Si on voit un début de transaction "''$db->begin"'' dans une boucle "for" alors on ne merge pas | Si on voit un début de transaction "''$db->begin"'' dans une boucle "for" alors on ne merge pas | ||
* il faudra sortir les transactions "begin" de la boucle ainsi que les "commit" et "rollback" | *il faudra sortir les transactions "begin" de la boucle ainsi que les "commit" et "rollback" | ||
'''Cas 4''' | '''Cas 4''' | ||
| Line 41: | Line 41: | ||
FIX SQL error | FIX SQL error | ||
* ici on se pose la question : | *ici on se pose la question : | ||
** est-ce qu'on peut corriger une requête SQL juste en rajoutant un champ ? | **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 | *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 | *le problème provenait du cumul d’un "DISTINCT" avec un "ORDER BY" qui n’était plus accepté par MySQL | ||
'''Cas 5''' | '''Cas 5''' | ||
| Line 50: | Line 50: | ||
Si on n’arrive pas à reproduire le bug alors on ne merge pas | 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 | *c'est le cas lorsque la base de données est corrompue | ||
=> on ne merge pas et au final c'est une réparation de la base de données qu'il faut faire | => on ne merge pas et au final c'est une réparation de la base de données qu'il faut faire | ||
| Line 58: | Line 58: | ||
On a ici un déplacement d’un paquet de code | On a ici un déplacement d’un paquet de code | ||
* le trigger doit être dans la méthode Update | *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 | *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 | => 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 | **pour corriger, il faut ajouter ''$notrigger = 1'' dans la page pour garder le même comportement qu'auparavant | ||
'''Cas 7''' | '''Cas 7''' | ||
| Line 71: | Line 71: | ||
Règles sur les fonctionnalités dépréciées : | Règles sur les fonctionnalités dépréciées : | ||
# mettre @deprecated dans les commentaires du code de la fonction à ne plus utiliser (sera supprimée dans plusieurs versions) | #mettre @deprecated dans les commentaires du code de la fonction à ne plus utiliser (sera supprimée dans plusieurs versions) | ||
# mettre un syslog pour mettre en déprécié lors de l'utilisation de la fonction (visible dans les logs de Dolibarr) | #mettre un syslog pour mettre en déprécié lors de l'utilisation de la fonction (visible dans les logs de Dolibarr) | ||
# mettre les informations dans le fichier « Changelog » | #mettre les informations dans le fichier « Changelog » | ||
[[File:Usecase7.png|center|thumb]] | [[File:Usecase7.png|center|thumb]] | ||
| Line 90: | Line 90: | ||
<nowiki>https://github.com/Dolibarr/dolibarr/pull/24481</nowiki> | <nowiki>https://github.com/Dolibarr/dolibarr/pull/24481</nowiki> | ||
* refactoring pour éviter « params » qui est utilisé dans les listes | *refactoring pour éviter « params » qui est utilisé dans les listes | ||
* on a un doute et l'on demande au développeur | *on a un doute et l'on demande au développeur | ||
* c’est un problème d’initialisation dans une boucle | *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 | *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 | => après plusieurs échanges avec le développeur, au final, c'est corrigeable en une ligne de code | ||
| Line 104: | Line 104: | ||
[[File:Usecase10.png|center|thumb]] | [[File:Usecase10.png|center|thumb]] | ||
* attention : on remplace ici un "continue" par quelque chose qui va changer systématiquement | *attention : on remplace ici un "continue" par quelque chose qui va changer systématiquement | ||
* problème de « checkbox » qui n’est pas coché ou pas de valeur par défaut | *problème de « checkbox » qui n’est pas coché ou pas de valeur par défaut | ||
** il fallait mettre une condition en mettant un champ caché sur le formulaire avec une valeur par défaut à 1 par exemple | **il fallait mettre une condition en mettant un champ caché sur le formulaire avec une valeur par défaut à 1 par exemple | ||
=> au final on a un bug qui a été corrigé en introduisant un autre bug (bug yoyo) et il fallait le corriger depuis le champ "checkbox" du formulaire : | => au final on a un bug qui a été corrigé en introduisant un autre bug (bug yoyo) et il fallait le corriger depuis le champ "checkbox" du formulaire : | ||
| Line 116: | Line 116: | ||
<nowiki>https://github.com/Dolibarr/dolibarr/pull/27050/commits/e8ddba34fabbdb8ea12088ad1e064f9c9d2c38da</nowiki> | <nowiki>https://github.com/Dolibarr/dolibarr/pull/27050/commits/e8ddba34fabbdb8ea12088ad1e064f9c9d2c38da</nowiki> | ||
* New trigger CRONJOB_ERROR | *New trigger CRONJOB_ERROR | ||
* ici on a une mauvaise utilisation des triggers et il aurait fallu utiliser des hooks | *ici on a une mauvaise utilisation des triggers et il aurait fallu utiliser des hooks | ||
* 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 | *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 | ||
=> on ne merge pas et il faudrait proposer une nouvelle PR avec l'utilisation de hooks | => on ne merge pas et il faudrait proposer une nouvelle PR avec l'utilisation de hooks | ||
| Line 126: | Line 126: | ||
<nowiki>https://github.com/Dolibarr/dolibarr/pull/29749/</nowiki> | <nowiki>https://github.com/Dolibarr/dolibarr/pull/29749/</nowiki> | ||
* ici on a un paquet de lignes | *ici on a un paquet de lignes | ||
=> au final il faut plus de commentaires de la part du développeur | => au final il faut plus de commentaires de la part du développeur | ||
==== Et pour les évolutions ==== | ====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 : | 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 | *respecter l'ergonomie de Dolibarr | ||
* se questionner sur | *se questionner sur | ||
** qui en a besoin ? | **qui en a besoin ? | ||
** est-ce une nouvelle règle fiscale qui doit-être mis en place rapidement ? | **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 | *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 pour en discuter en amont de la fonctionnalité à apporter dans le projet Dolibarr. | 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. | ||
<br /> | <br /> | ||