[TECH] Linter les titres de PR #5445
Conversation
|
|
|
Le titre |
|
L'action est de nouveau Skipped car en draft. |
|
Erreur sur le titre car pas de préfixe contexte du style
|
|
Titre invalide car espace manquant après le contexte.
|
yannbertrand
changed the title
|
L'action a tourné même si la PR est fermée, il faut filtrer ça. |
yannbertrand
changed the title
|
J'ai eu besoin de retrigger le workflow en changeant le titre puisque les nouveaux commits ne retrigger pas 🙃 |
yannbertrand
changed the title
|
Le workflow est Skipped ✌️
|
|
Le format de préfixe est invalide. |
yannbertrand
changed the title
|
C'est toujours Skippé car en draft :). On est bon ! |
|
On peut le mettre là non ? |
| steps: | ||
| - uses: Slashgear/action-check-pr-title@v4.0.0 | ||
| with: | ||
| regexp: '\[[A-Z]+\] .+' |
There was a problem hiding this comment.
| regexp: '\[[A-Z]+\] .+' | |
| regexp: '\[[A-Z]+\] .+\.' |
Les titres des PR doivent normalement contenir un . à la fin. Peut-être peut-on l'ajouter facilement ?
There was a problem hiding this comment.
Je ne suis pas pour car ce point n'apporte rien IMO (perso je l'ai jamais mis)
There was a problem hiding this comment.
Le point est présent dans le Contributing.md, et permet une cohérence visuelle entre les PR (certaines finissent avec rien, d'autres avec des parenthèses liés au numéro de ticket).
IMO, ca reste une bonne pratique peu coûteuse.
There was a problem hiding this comment.
Je vois dans les PRs ouvertes en ce moment qu'il y en a beaucoup qui n'ont pas de point. Je ne comprend pas l'argument de la cohérence visuelle ?
Je préfère ne pas trancher le débat dans cette PR si l'avis est partagé en tout cas, donc de laisser libre ou non de le mettre. Ça n'empêchera pas d'avoir ce débat dans une autre PR comme pour d'autres contraintes que l'on souhaiterait ajouter. WDYT ?
There was a problem hiding this comment.
Je suis également pour mettre un point à la fin, je ne le fais pas pour autant à chaque fois. Pour moi un Linter sert à s'assurer/imposer une rigueur sur un format donc ça collerait.
Après je serais d'accord que cette PR n'a pas pour vocation d'imposer ou affirmer une nouvelle règle mais de vérifier une règle déjà bien partagée donc on pourrait passer la PR sans ça.
There was a problem hiding this comment.
Pour la cohérence, je pensais en particulier au Changelog: https://github.com/1024pix/pix/blob/dev/CHANGELOG.md
There was a problem hiding this comment.
Ah ok merci pour la précision :)
On pourrait l'ajouter automatiquement dans la génération du changelog car c'est nous qui l'implémentons ! 😄
On en rediscute dans une autre PR 😉
There was a problem hiding this comment.
Point ajouté automatiquement au changelog avec 1024pix/pix-bot#256
| steps: | ||
| - uses: Slashgear/action-check-pr-title@v4.0.0 | ||
| with: | ||
| regexp: '\[[A-Z]+\] .+' |
There was a problem hiding this comment.
Si on se concentre sur la partie Tag, est-ce qu'on ne devrait pas plutôt lister les types aussi de tags ?
| regexp: '\[[A-Z]+\] .+' | |
| regexp: '\[(TECH|FEATURE|BUGFIX|DOC)\] .+\.' |
Qu'en pensez-vous ?
There was a problem hiding this comment.
La liste des tags est dispo dans le contributing.
Au choix, soit on contraint, soit on laisse libre avec le [A-Z]+ pour inclure DOC et tous les autres mais qui ne servent pas pour générer le changelog (donc tombent dans la section "Autres").
Sur Pix UI certains utilisent [BREAKING CHANGE], je ne pense pas que ce soit la bonne approche, mais c'est un constat qu'on a d'autres tags utilisés au global sur tous les projets.
IMO autant laisser libre sachant que ça n'a pas d'impact.
There was a problem hiding this comment.
Actuellement, dans Autres, on peut voir :
DOC, CLEANUP, TASK, PERFORMANCE, A11Y, DOCUMENTATON (en plus de DOC), DX, BUG (au lieu de BUGFIX), ADMIN (donc l'app), API, etc.
Pour une première PR, permettre tout me parait OK, mais la fonctionnalité permettrait d'éviter d'avoir des doublons différents, ou alors des info HS (API, etc..).
Ca demande peut être par contre de modifier le contributing pour lister les quelques tags manquants qu'on souhaiterait ajouter.
Certains utilisent deux tags [BUGFIX] [API]: la regex ne l'empêche pas.
There was a problem hiding this comment.
M'étant plusieurs fois posé la question pour de l'accessibilité et du Pix UI, je serais pour cadrer les tags.
There was a problem hiding this comment.
Voir #5451 pour une discussion sur ce sujet en particulier :)
|
J'ai revue ma copie sur la syntaxe de la regex qui ne précise pas que le tag doit être en début de ligne, je propose plutôt ça : (avec un espace à la fin) Qui sera à tester :) EDIT : testé avec regex101 et validé sur ce titre de PR ✅ |
|
Happy to see that this action could be useful for you! 🫶 |
yannbertrand
added
Tech Review OK
🚀 Ready to Merge
and removed
👀 Tech Review Needed
labels
pix-service-auto-merge
force-pushed
the
tech-lint-pr-titles
branch
3 times, most recently
from
c77020b
to
a6e9450
Compare
pix-service-auto-merge
force-pushed
the
tech-lint-pr-titles
branch
from
a6e9450
to
59d0062
Compare
🎄 Problème
Notre guide de contribution impose un format de titre de PR. La validation du respect de formatage n'est validé que par l'humain aujourd'hui.
🎁 Proposition
Implémenter une GitHub Action qui valide cette règle. Je propose de valider uniquement qu'un contexte soit donné entre crochets au début du titre :
En effet, il est important pour la génération du changelog. Par contre il n'y a pas nécessairement de ticket Jira associé, donc pas obligatoire de faire cette validation IMO.
🌟 Remarques
J'ouvre une PR sur https://github.com/Slashgear/action-check-pr-title pour proposer un message d'erreur plus lisible quand l'action fail.
🎅 Pour tester
Lire le fil de discussion de cette PR où je pense avoir tout testé :)