Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

đŸ‘©â€đŸ’» Add pre-commit hooks #131

Draft
wants to merge 5 commits into
base: beta
Choose a base branch
from

Conversation

Aeris1One
Copy link
Collaborator

@Aeris1One Aeris1One commented Feb 20, 2023

Ajout de quelques pre-commit hooks.

Ce sont des vĂ©rifications qui tournent cĂŽtĂ© dĂ©veloppeur et empĂȘchent le commit si elles ne passent pas.

Ces vérifications sont :

  • la validitĂ© des fichiers .json et .yaml
  • le style via Black (avec autocorrection, si un commit est refusĂ© par Black, il suffit de refaire le commit)
  • les sauts de lignes en fin de fichier (idem)
  • le message de commit correspond au format Gitmoji + Conventional Commit (:emoji: [type](scope): <message>)

Si cette PR est merge, il sera nécessaire pour tous les développeurs d'utiliser la commande pre-commit install aprÚs avoir installé les dépendances de Gipsy (dans le venv le cas échéant) afin d'installer les hooks dans leur .git local.

@ascpial ascpial changed the title Add pre-commit hooks đŸ‘©â€đŸ’» Add pre-commit hooks Feb 20, 2023
@ascpial
Copy link
Contributor

ascpial commented Feb 20, 2023

Est-ce que les commits au format đŸ‘©â€đŸ’» docs(misc): documented things passe ?

De la mĂȘme maniĂšre est-ce que les vĂ©rifications sont faites pour les commits crĂ©Ă©s sur Github ?

@ascpial ascpial self-requested a review February 20, 2023 12:43
@Aeris1One Aeris1One marked this pull request as draft February 20, 2023 12:43
@Aeris1One
Copy link
Collaborator Author

Pardon, j'aurais dû la mettre en draft, je n'ai pas terminé

@Aeris1One
Copy link
Collaborator Author

J'ai oublié de le commit en séparé mais je viens d'ajouter un hook PyLint.

⚠ ATTENTION
Une fois cette PR merge, un commit sur un fichier sera refusé tant que des erreurs seront soulevées par PyLint sur celui-ci

Je viens Ă©galement d'ajouter une configuration pour https://pre-commit.ci qui s'occupera de faire tourner les hooks sur les PRs et d'autoupdate les hooks chaque mois.
Il faut pour cela qu'un administrateur de l'organisation Gunivers autorise à https://pre-commit.ci l'accÚs (restreint) au dépÎt Gipsy, j'en ais fait la demande, il ne manque que l'autorisation.

@Aeris1One Aeris1One marked this pull request as ready for review February 20, 2023 13:08
@VForiel
Copy link
Contributor

VForiel commented Feb 20, 2023

Une fois cette PR merge, un commit sur un fichier sera refusé tant que des erreurs seront soulevées par PyLint sur celui-ci

Chiant/20 ça

@Aeris1One Aeris1One requested review from a team, VForiel and ZRunner and removed request for a team February 20, 2023 13:39
@ZRunner
Copy link
Contributor

ZRunner commented Feb 20, 2023

Une fois cette PR merge, un commit sur un fichier sera refusé tant que des erreurs seront soulevées par PyLint sur celui-ci

Chiant/20 ça

Yup, mĂȘme avis. Autant c’est un bon point pour refuser une MR, autant selon moi ça ne devrait pas empĂȘcher de commit

Deuxiùmement, y a-t-il un hook permettant de s’assurer qu’un commit ne contienne pas de token d’authentification (peu importe de quel service) ? Ça ne devrait pas arriver mais ça pourrait nous sauver un jour ou l’autre.

@ascpial
Copy link
Contributor

ascpial commented Feb 25, 2023

Tant qu'on y est, il faudra aussi ajouter la commande pour installer les dépendances du pre-commit hook dans le fichier setup.py

@Aeris1One
Copy link
Collaborator Author

Je ne pense pas que ce soit souhaitable. Les pre-commits hooks sont pour les contributeurs lĂ  oĂč le setup.py est prĂ©vu pour ceux qui veulent utiliser le bot.
Sans compter qu'il est possible d'utiliser le bot hors de git (en téléchargeant les sources en .zip tout simplement) et donc la commande ne servirait à rien et sortirai une erreur.

@ascpial
Copy link
Contributor

ascpial commented Feb 27, 2023

Yup, mĂȘme avis. Autant c’est un bon point pour refuser une MR, autant selon moi ça ne devrait pas empĂȘcher de commit

@Aeris1One, y a-t-il moyen d'afficher seulement une alerte si il y a des erreurs pylint ?
Je penses aussi que bloquer le commit n'est pas une bonne idée.

@ascpial
Copy link
Contributor

ascpial commented Mar 14, 2023

Au moment de merge, je conseilles que l'on recrĂ©Ă© une PR avant de merge, puisque l'auto fix empĂȘche de merge.

@ascpial ascpial added this to the v1.5 milestone Mar 14, 2023
@ascpial ascpial added the workflow Improvments to the workflow (CI, actions...) label Mar 16, 2023
@Aeris1One
Copy link
Collaborator Author

Aeris1One commented Mar 25, 2023

Je propose de retirer le bloquage des linters (retirer PyLint) et ajouter un check regex pour empĂȘcher de push un token par erreur


Yup, mĂȘme avis. Autant c’est un bon point pour refuser une MR, autant selon moi ça ne devrait pas empĂȘcher de commit

Cela dit je pense que bloquer les commits est une bonne idĂ©e pour ĂȘtre sĂ»r que tout l'historique est propre, pas seulement les derniers commits de chaque PR ce qui permettrait quand nous remontons dans le temps de ne pas avoir Ă  choisir avec 4 commits sur 5 qui sont "sales"

@ascpial
Copy link
Contributor

ascpial commented Mar 25, 2023

Je suis d'accord, mais il faut indiquer les erreurs pylint et demander confirmation si les checks pylint Ă©chouent.
Dans mon cas, pylint met 20s Ă  vĂ©rifier start.py, et je ne peux pas l'utiliser avec vscode car sinon il crash, il faut donc qu'ils soient tout de mĂȘme affichĂ©.

Je propose aussi de créer une page dans la documentation dédiée à la mise en place de l'environnement de développement.

@Aeris1One Aeris1One marked this pull request as draft March 25, 2023 10:46
@ascpial
Copy link
Contributor

ascpial commented Mar 25, 2023

Les linters ne sont plus dans les actions github mais sont inclus directement avec pre-commit ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow Improvments to the workflow (CI, actions...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants