Améliore template de pull request #962
Conversation
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| @@ -11,7 +11,17 @@ Merci de contribuer à OpenFisca ! Effacez cette ligne ainsi que, pour chaque li | |||
|
|
|||
| Ces changements _(effacez les lignes ne correspondant pas à votre cas)_ : | |||
|
|
|||
| - Impactent l'API publique d'OpenFisca France (par exemple renommage ou suppression de variables). | |||
| - Breaking-change de l'API publique d'OpenFisca France (par exemple renommage ou suppression de variables). | |||
There was a problem hiding this comment.
L'utilisation de termes anglais est dommage.
Que penses-tu de :
Impactent (sans rétro-compatibilité) l'API publique…
There was a problem hiding this comment.
Oui, n'oublions pas que le template sert principalement à aider des contributeurs qui sont novices dans le maintien de librairie open source à qualifier la nature de leurs changement.
Evitons le jargon
There was a problem hiding this comment.
J'ai du mal à comprendre en quoi ajouter ce détail va aider des utilisateurs novices à n'effacer la ligne que dans le cas où c'est pertinent, même en français. Je préfère avoir un faux positif de non-rétro-compatibilité et le corriger à la revue que laisser passer un breaking change.
There was a problem hiding this comment.
Merci pour les retours, ce n'était pas un sujet de périmètre mais de vocabulaire, le mot « impactent » étant un peu difficile à saisir. D'autres options :
- Modifient
- Introduisent un changement à
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| Tips avant de demander une review : | ||
|
|
||
| - [ ] J'ai fait un rebase à partir de [`master`](https://git-scm.com/docs/git-rebase) |
There was a problem hiding this comment.
Je ne suis pas sûr que ça vaille le coup d'imposer ça aux contributeurs.
D'une part, c'est une manipulation qui n'est pas triviale pour ceux qui débutent avec git.
D'autre part, de toute façon, entre le moment où la revue est demandée et celui où elle la PR est approuvée, il est très probable qu'une autre branche ait été mergée, et donc qu'un rebase soit à nouveau nécessaire.
There was a problem hiding this comment.
Oui d'accord avec toi, mais je voudrais comme même donner un « tip », plutôt que de l'imposer. Peut-être cela serait mieux de dire « faire un rebase régulièrement » ou « faîtes de pull requests petites ».
D'une part, c'est une manipulation qui n'est pas triviale pour ceux qui débutent avec git.
Cela c'est un problème plus général : pour quelqu'un qui corrige un typo dans la doc, et qui créé une pull request à partir d'un fork, et que sa branch devienne outdated, comment pourrait-on faire pour que cela soit simple de pouvoir merger sans devoir faire un rebase ?
There was a problem hiding this comment.
comment pourrait-on faire pour que cela soit simple de pouvoir merger sans devoir faire un rebase
Sans faire de rebase c'est un no-go pour moi, ça veut dire remplacer par des merge upstream et ça devient vite très sale. Après, ça pourrait être plus simple de rebaser via l'UI de Github 😞 : isaacs/github#88.
Peut-être cela serait mieux de dire « faire un rebase régulièrement »
Régulièrement je ne sais pas, on ne veut pas non plus encourager des rebases en milieu de revue. Donc a priori on rebase seulement à deux moments : à l'ouverture avant la demande de review, et après l'approbation.
There was a problem hiding this comment.
ça pourrait être plus simple de rebaser via l'UI de Github
Attention, l'implémentation du rebase de l'UI de GitHub est un rebase + fast-forward, là où la politique qu'on a mise en place est un rebase + merge --no-ff.
En termes de lisibilité de l'historique, je trouve ça très différent. À noter également que dans 90% des cas ce sera impossible à faire puisqu'il y aura un conflit sur le change log et sur le numéro de version.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| - [ ] J'ai fait un rebase à partir de [`master`](https://git-scm.com/docs/git-rebase) | ||
| - [ ] J'ai mis à jour le [`CHANGELOG.md`](https://github.com/openfisca/openfisca-france/blob/master/CONTRIBUTING.md#format-du-changelog) | ||
| - [ ] J'ai mis à jour les références législatives (si cela s'applique) |
There was a problem hiding this comment.
Dans quel cas cela s'applique ?
There was a problem hiding this comment.
C'est peut-être :
J'ai documenté ma contribution avec des références législatives.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| @@ -1,4 +1,4 @@ | |||
| Merci de contribuer à OpenFisca ! Effacez cette ligne ainsi que, pour chaque ligne ci-dessous, les cas ne concernant pas votre contribution :) | |||
| Merci de contribuer à OpenFisca ! **Effacez cette ligne ainsi que, pour chaque ligne ci-dessous, les cas ne concernant pas votre contribution** :) | |||
There was a problem hiding this comment.
Le Markdown n'est de toute façon pas rendu en aperçu dans le template, les astérisques ne permettront donc pas de faire apparaître cette phrase en gras.
There was a problem hiding this comment.
De toute façon, il y a déjà du markdown dans la ligne 12 :
(effacez les lignes ne correspondant pas à votre cas)
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| @@ -11,7 +11,17 @@ Merci de contribuer à OpenFisca ! Effacez cette ligne ainsi que, pour chaque li | |||
|
|
|||
| Ces changements _(effacez les lignes ne correspondant pas à votre cas)_ : | |||
|
|
|||
| - Impactent l'API publique d'OpenFisca France (par exemple renommage ou suppression de variables). | |||
| - Breaking-change de l'API publique d'OpenFisca France (par exemple renommage ou suppression de variables). | |||
There was a problem hiding this comment.
J'ai du mal à comprendre en quoi ajouter ce détail va aider des utilisateurs novices à n'effacer la ligne que dans le cas où c'est pertinent, même en français. Je préfère avoir un faux positif de non-rétro-compatibilité et le corriger à la revue que laisser passer un breaking change.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| - - - - | ||
|
|
||
| Tips avant de demander une review : |
There was a problem hiding this comment.
Trop d'anglais, et la notion même de "demande de review" n'est pas forcément évidente pour un premier contributeur : garder la confusion Pull Request / Peer Review me semble simple. Si la personne est déjà en train d'ouvrir une PR, je ne crois pas qu'on doive lui suggérer que demander une review est un processus indépendant.
There was a problem hiding this comment.
En plus, il me semble qu'il faut être membre de l'organisation pour pouvoir demander une revue. Donc on peut s'attendre à ce qu'une minorité non-négligeable de contributeurs ne puisse pas le faire.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| Tips avant de demander une review : | ||
|
|
||
| - [ ] J'ai fait un rebase à partir de [`master`](https://git-scm.com/docs/git-rebase) | ||
| - [ ] J'ai mis à jour le [`CHANGELOG.md`](https://github.com/openfisca/openfisca-france/blob/master/CONTRIBUTING.md#format-du-changelog) |
There was a problem hiding this comment.
Oui mais c'est un goulet d'étranglement aujourd'hui, car prend beaucoup de temps. L'idée c'est de le prendre en compte à l'avance.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| - [ ] J'ai fait un rebase à partir de [`master`](https://git-scm.com/docs/git-rebase) | ||
| - [ ] J'ai mis à jour le [`CHANGELOG.md`](https://github.com/openfisca/openfisca-france/blob/master/CONTRIBUTING.md#format-du-changelog) | ||
| - [ ] J'ai mis à jour les références législatives (si cela s'applique) | ||
| - [ ] J'ai crée / modifié les tests qui correspondent au changements (si cela s'applique) |
There was a problem hiding this comment.
Limitons le nombre de points de décision : je ne suis pas sûr que "si cela s'applique" aide beaucoup.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| - [ ] J'ai mis à jour le [`CHANGELOG.md`](https://github.com/openfisca/openfisca-france/blob/master/CONTRIBUTING.md#format-du-changelog) | ||
| - [ ] J'ai mis à jour les références législatives (si cela s'applique) | ||
| - [ ] J'ai crée / modifié les tests qui correspondent au changements (si cela s'applique) | ||
| - [ ] J'ai augmenté le numéro de version dans [`setup.py`](https://github.com/openfisca/openfisca-france/blob/master/setup.py) |
There was a problem hiding this comment.
Ok pour essayer si vous pensez que c'est un point de frustration récurrent, mais je suis dubitatif sur l'effet. SemVer n'est pas évident à comprendre, surtout pour des novices, et la probabilité que le numéro de version doive être à nouveau incrémenté lors du merge en raison d'autres merges ayant eu lieu en parallèle est élevée.
L'objectif en demandant de choisir entre Évolution du système socio-fiscal. | Amélioration technique. | Correction d'un crash. | Changement mineur. consistait à aider les mainteneurs à incrémenter le numéro de version correctement. En faisant des retours sur le sujet à chaque fois que ça n'est pas fait, on peut petit à petit faire monter en compétence les contributeurs.
|
Je ne suis pas sûr de comprendre suffisamment bien quel est le problème plus général.
Je peux inférer deux objectifs potentiels :
1. Réduire la charge administrative sur les mainteneurs.
2. Accélérer la livraison des PR.
Et j'entends deux solutions potentielles exprimées :
A. Externaliser le rebase sur les contributeurs.
B. Réduire la taille des PR.
Avec cette PR proposant de pousser vers A.
Malheureusement, j'ai le sentiment que l'intensité de ces deux problèmes sont inversement proportionnelles (i.e. je crois qu'externaliser le rebase ralentit la livraison), ce qui s'explique par le faible niveau moyen en Git des contributeurs. Le coût d'une opération comme rebase est très élevé pour le contributeur moyen (au point qu'il y a un risque de perte de code, d'introduction d'erreur ou d'abandon de contribution) alors qu'il est plusieurs ordres de grandeur plus faible pour les mainteneurs.
Je suis convaincu que la solution A est trop risquée car elle augmente démesurément le coût de la contribution. Je crois qu'on ne devrait l'appliquer que sur les contributeurs les plus expérimentés.
Et cela fait longtemps que l'on tente systématiquement de mettre en place la solution B. Malheureusement, vu le coût administratif fixe du changelog, nous avons des _incentives_ perverses. Je ne suis pas certain que les contributeurs comme l'IPP, par exemple, aient été particulièrement convaincus que cela accélérait la livraison de leurs PR.
Ma conclusion est que les deux solutions sont pertinentes, mais qu'elles ne pourront pas être effectivement implémentées en modifiant le template de PR. Inversement, je vois des risques majeurs à demander la mise en œuvre de A sans accompagnement personnalisé. Je considère donc que le RoI du changement proposé est <0.
Ma dernière expérience d'accompagnement m'a montré que même le template actuel de PR est ambigu et le jeu de contraintes pas évident à comprendre pour un nouveau contributeur.
- - - -
Je vois deux autres solutions possibles aux problèmes listés plus haut :
C. Augmenter progressivement le nombre de contraintes appliquées aux contributeurs, en créant l'équivalent d'un « budget de relecture » que les mainteneurs passent à examiner des contributions. Il faut construire cela de manière très attentive pour bien montrer qu'il s'agit de faire monter en compétence et non de refuser l'accompagnement.
D. Abandonner la lisibilité de l'historique et basculer vers un système de merge autorisant le merge upstream.
Je ne suis pas convaincu par l'une ou l'autre :(
Enfin, si l'un des problèmes non explicités est les conflits rencontrés par les mainteneurs au rebase, il existe des solutions techniques pour limiter ces effets que je serai heureux de présenter si besoin.
|
|
Bien vu, +1 pour « modifient ».
|
Ça m'intéresse ! |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| Quelques conseils à prendre en compte : | ||
|
|
||
| - Documenter ma contribution avec des références législatives. | ||
| - Mettre à jour ou ajouter les tests qui correspondent à ma contribution. |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| Quelques conseils à prendre en compte : | ||
|
|
||
| - Documenter ma contribution avec des références législatives. |
There was a problem hiding this comment.
Je trouve que l'infinitif + première personne du singulier (documenter ma contribution) passe mal après l'impératif (effacez les lignes).
Je n'ai rien à proposer tout de suite, désolé, mais il doit être possible de trouver une formulation plus cohérente.
There was a problem hiding this comment.
J'aime bien le format cases à cocher, par exemple pour brew.
There was a problem hiding this comment.
Dans ce cas là, il faudrait poser plutôt des questions... Voici quelques questions à prendre en compte ... Cela ne marche pas très bien...
|
Je suis plutôt en phase sur le fond avec la version actuelle des tips, merci pour les adaptations 🙂. |
Pour détecter les problèmes au plus tôt, |
Il y a deux cas ici :
c'est intéressant.
Aujourd'hui Github propose d'intégrer des changements upstream on utilisant trois stratégies différentes : rebase, squash et merge. Peut-être qu'il faudrait y jeter un oeil. Cela dit, c'en est une solution conditionnée à la taille des PR IMHO, car les gros PRs seront toujours impossibles de rebaser automatiquement. J'ai enlevé le « tip » pour faire rebase, car en réfléchissant c'est un art difficile et difficile à bien maîtriser. Vous m'avez convaincu que c'est un no-go de l'externaliser (au moins pour les arrivant·e·s).
😃 Tell me more!
C'est intéressant, mais je ne sais pas à quel point c'est trop intégré avec git-flow (je ne suis pas hyper à l'aise avec les workflows « normatifs »). De toutes les manières, tant le « budget de relecture » comme git-octopus vont dans le sens des nudges pour augmenter progressivement le nombre de contraintes appliquées aux contributeurs. Une dernière idée serait de favoriser le partage du work-in-progress, pour faire un accompagnement plus dans la durée et pas seulement à la fin, mais cela ne s'appliquerait qu'aux contributeurs qui font déjà partie de la communauté. |
Ça ne l'est pas : tu donnes N branches,
Attention, le rebase proposé n'est pas forcément celui auquel vous pensez, et je crois que de toute façon l'UI ne fonctionnera pas pour l'intégration finale car il y aura toujours un conflit sur changelog et setup.py, cf. #962 (comment). |
|
Sur openfisca/openfisca-doc#108, et en général les contributeurs externes à l'orga github, je ne sais pas si le seul problème est le rebase. J'ai l'impression que les tests Circle ne sont pas déclenchés, et que c'est aussi ça qui bloque le merge.
EDIT: Plus simple, il est possible d'éditer la branche (y compris force-push) du repo d'un contributeur externe s'il existe une pull request ouverte depuis cette branche sur un de nos repos. La seule chose à faire pour pouvoir rebaser est donc de récupérer sa branche, en ajoutant temporairement son remote sur notre version locale du dépôt. Je viens de le faire pour openfisca/openfisca-doc#108, ça m'a pris 2 minutes: git remote add pa git@github.com:pachevalier/openfisca-doc.git
git fetch --all
git checkout patch-1
git rebase origin/master
git push --force |
|
Il y a un point de config Circle à changer, je m'en occupe:
|
|
Sur #881, je ne sais pas si on a eu des contacts via d'autres medias que Github avec le contributeur, mais je ne suis pas sûr que le rebase soit la plus grosse difficulté, relativement à:
|
Oui contact IRL pendant la démo, où il nous a montré sont travail et nous l'avons invité à créer la pull request.
👍
👍
Oui. Dans ce cas, le gros du travail avait déjà été fait. |
| Quelques conseils à prendre en compte : | ||
|
|
||
| - [ ] Jetez un coup d'œil au [guide de contribution](CONTRIBUTING.md). | ||
| - [ ] Regardez s'il n'y a pas une [proposition introduisant ces mêmes changements](https://github.com/openfisca/openfisca-france/pulls). |
There was a problem hiding this comment.
Je crois pas que le problème se soit jamais posé jusqu'à aujourd'hui
There was a problem hiding this comment.
Cela c'est fléché collaboration avec la MSA poke @guillett
bonjourmauko
force-pushed
the
update-pull-request
branch
from
319a201
to
677f83c
Compare
L'objectif est de améliorer la qualité du processus de review, et par ce biais de le rendre plus rapide.
bonjourmauko
force-pushed
the
update-pull-request
branch
from
677f83c
to
0ada83b
Compare
.github/PULL_REQUEST_TEMPLATE.md.Ces changements :