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

Corrections diverses dans le formatage des fichiers sources #6484

Merged
merged 6 commits into from
Sep 2, 2023

Conversation

philippemilink
Copy link
Member

Tout part de la revue de code que j'ai faite pour cette PR : #6482. Le commit contient un changement à la fin du fichier, qui (a priori) ne change rien et n'a rien à voir avec le sujet de la PR. Ce changement vient de l'absence de caractère \n à la fin de la dernière ligne du fichier. L'éditeur l'a rajouté automatiquement en faisant la modification principale de la PR, car c'est de cette façon que la dernière ligne d'un fichier doit se terminer.

J'ai donc voulu corriger tous les fichiers qui ne terminent pas par le bon caractère. Pour automatiser cette correction et la vérification de cette règle pour les futures modifications, j'ai commencé par ajouter des tâches à exécuter dans le pre-commit hook configuré dans le fichier .pre-commit-config.yaml.

Le problème de pre-commit, c'est qu'il n'est exécuté que en local chez chaque développeur.se, donc on ne peut avoir aucune certitude qu'il a été exécuté. J'ai donc modifié la CI pour qu'elle exécute pre-commit, plutôt que seulement black pour vérifier le format des fichiers Python.

En lançant pre-commit sur tous les fichiers, pyupgrade se plaignait qu'il y a des changements à faire dans le fichier script/generate_release_summary.py. Mais on n'utilise plus ce fichier, donc j'en ai profité pour le supprimer (plus de justifications dans le message du commit correspondant). J'ai corrigé ce que pyupgrade reprochait à doc/source/conf.py.

Ensuite, en voyant tous les fichiers modifiés pour corriger les infractions aux nouvelles règles instaurées, j'ai remarqué certains fichiers qui ne devraient pas être dans le dépôt :

  • errors/css/main.css : ce fichier est généré par Gulp à partir de errors/scss/main.scss, donc je l'ai supprimé et j'en ai profité pour mieux intégrer sa construction dans le pipeline Gulp ;
  • le dossier export-assets contenait des fichiers de polices et des fichiers liés à Pandoc, ces fichiers n'étant pas utilisés, je les ai supprimés (au passage, j'ai supprimé les mentions de Pandoc dans la doc).

Question

Maintenant que pyupgrade est satisfait de l'ensemble des fichiers Python dans le dépôt, est-ce que ce ne serait pas l'occasion de le supprimer des checks réalisés lors par pre-commit ? Si on tente d'introduire du code Python 2, black devrait hurler, non ?

Contrôle qualité

Vu la quantité de fichiers changés et les changements réalisés, relire tous les changements n'est pas vraiment envisageable... Je vous conseille de regarder les différents commits, et juste regarder les changements principaux.

La CI contente devrait être suffisant pour s'assurer qu'il n'y a pas de régression.

Merge

Puisque ces commits portent sur des points assez différents, ne pas merger en squashant les commits.

@coveralls
Copy link

coveralls commented Apr 1, 2023

Coverage Status

coverage: 88.393%. remained the same when pulling e6c8e5e on philippemilink:fix-endfiles into b6fd744 on zestedesavoir:dev.

@philippemilink
Copy link
Member Author

(Le check Lint back-end apparaît toujours comme Required dans la liste des checks néessaires pour pouvoir merger la PR, c'est un paramètre à changer dans les paramètres du dépôt. Approuvez seulement la PR, je ferai le merge en bypassant les checks requis et je modifierai les paramètres du dépôt.)

@Situphen
Copy link
Member

Situphen commented Apr 1, 2023

Maintenant que pyupgrade est satisfait de l'ensemble des fichiers Python dans le dépôt, est-ce que ce ne serait pas l'occasion de le supprimer des checks réalisés lors par pre-commit ? Si on tente d'introduire du code Python 2, black devrait hurler, non ?

Je pense qu'il y a un intérêt à continuer de l'utiliser car tout le monde ne connaît pas ou n'a pas l'habitude d'utiliser les nouvelles fonctionnalités de Python qui permettent d'avoir un code plus compréhensible. Je pense notamment aux f-strings et à super() sans argument, mais il y en a beaucoup d'autres. À ma connaissance, black ne va pas râler tant que le code est du Python 3 correct donc pyupgrade reste nécessaire.

@philippemilink
Copy link
Member Author

Maintenant que pyupgrade est satisfait de l'ensemble des fichiers Python dans le dépôt, est-ce que ce ne serait pas l'occasion de le supprimer des checks réalisés lors par pre-commit ? Si on tente d'introduire du code Python 2, black devrait hurler, non ?

Je pense qu'il y a un intérêt à continuer de l'utiliser car tout le monde ne connaît pas ou n'a pas l'habitude d'utiliser les nouvelles fonctionnalités de Python qui permettent d'avoir un code plus compréhensible. Je pense notamment aux f-strings et à super() sans argument, mais il y en a beaucoup d'autres. À ma connaissance, black ne va pas râler tant que le code est du Python 3 correct donc pyupgrade reste nécessaire.

Ok, donc on le garde.

@philippemilink philippemilink force-pushed the fix-endfiles branch 2 times, most recently from 9d4e7ca to dd9488d Compare April 2, 2023 14:03
@philippemilink philippemilink force-pushed the fix-endfiles branch 2 times, most recently from a340080 to 02e6bfa Compare June 4, 2023 09:40
@Arnaud-D
Copy link
Contributor

Arnaud-D commented Jun 4, 2023

On pourra probablement se débarasser de pyupgrade si on se met à utiliser un véritable linter Python en plus de black qui ne fait effectivement que le style.

Copy link
Contributor

@Arnaud-D Arnaud-D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai parcouru les fichiers assez longuement et rien de bizarre ne m'a sauté aux yeux.

QA OK ✔️

Edit. : Par contre, il y a un truc qui ne passe pas dans la CI, je ne sais pas quel est le souci.

@Situphen
Copy link
Member

Par contre, il y a un truc qui ne passe pas dans la CI, je ne sais pas quel est le souci.

La CI ne passe car cette PR renomme la tâche Github Actions lint-back en lint, donc la tâche lint est bien lancée et la tâche lint-back n'est pas lancée. Or la tâche lint-back est marquée dans les paramètres du dépôt comme étant obligatoire, donc ça bloque ! @philippemilink va pouvoir corriger ça aisément

Et intégre mieux sa construction au pipeline Gulp.
On n'utilise plus ce script, il repose sur les milestone de GitHub qu'on
n'utilise pas, le code Python utilise d'anciens standards, GitHub
maintenant génère lui-même une liste des commits lors de la création
d'un release.
Et retire toute mention à Pandoc dans la documentation (Pandoc a été
remplacé par ZMarkdown depuis longtemps).
Les ajouts au pre-commit hook vérifient :
- l'absence d'espaces inutiles à la fin des lignes
- que les fichiers terminent par un unique caractère "nouvelle ligne"
- que les nouvelles lignes sont codées avec LF et non CRLF
Ces règles correspondent à ce qui est déclaré dans .editorconfig.

Le commit corrige en plus toutes les infractions à ces règles.
@philippemilink philippemilink merged commit 489a95d into zestedesavoir:dev Sep 2, 2023
8 checks passed
@philippemilink philippemilink deleted the fix-endfiles branch September 2, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants