Skip to content

Conversation

@AlexisGauthierAtKronos
Copy link
Contributor

Passage au linter Ruff.

La job code-style est maintenue. On y utilise maintenant Ruff avec seulement le sous-ensemble de règles correspondant à celles qui étaient couvertes par pycodestyle.

On ajoute un nouvel ensemble de règles via la job future-code-style.
Ces règles ne devraient pas être bloquantes dans le workflow. Éventuellement, lorsque les projets seront prêts, on pourra en transférer vers code-style.

- name: Run future codestyle
id: run_future_codestyle
working-directory: ${{ inputs.working-directory }}
run: ${{ needs.setup.outputs.dependencies-manager }} run ruff check --select ANN --select F --select I --select N --select PD --ignore ANN101 --ignore ANN102 --ignore ANN401 --ignore PD015 --ignore PD901 --statistics ./
Copy link
Contributor

Choose a reason for hiding this comment

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

Pour faire progresser une règle vers la job code-style, tous les projets vont devoir fixer les erreurs liées à cette règle en même temps.

La job code-style devrait utiliser la config pyproject.toml dans le projet.
Pour la job future-code-style, on pourrait créer un fichier de config ruff nommé future-ruff.toml dans les projets python qui contiendrait les règles que tu énumères ici. (la job ne devrait pas échouer si le fichier n'existe pas). On peut utiliser l'option --config pour spécifier le fichier ruff.

Chaque projet serait donc autonome dans la progression du code style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔
Tant qu'à laisser une certaine indépendance d'un projet à l'autre et tant qu'à lire les règles spécifiques à un projet, peut-être que ça ne serait pas nécessaire de séparer le check en deux. On pourrait avoir simplement code_style.

Dans le python-quality-checks.yml au lieu d'avoir :

run: ${{ needs.setup.outputs.dependencies-manager }} run ruff check --select E --select W --statistics ./

On pourrait mettre :

run: ${{ needs.setup.outputs.dependencies-manager }} run ruff check --extend-select E --extend-select W --statistics ./

Ainsi, on y garantie l'utilisation minimale des règles E et W et ces règles viendraient s'ajouter aux règles du fichier .toml fourni qui sont spécifiques au projet.

J'ai quelques essais à effectuer, mais j'aurais tendance à y aller dans cette direction si ça te conviens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tout en ayant la ligne ci-haut mentionnée dans le workflow

run: ${{ needs.setup.outputs.dependencies-manager }} run ruff check --extend-select E --extend-select W --statistics ./

J'ai essayé dans un projet :

  • select = [] sans erreurs dans le projet et tout passe.
  • select = [] et avec une erreur E501 (line too long) ça a bien été flaggué (donc extend-select E est utilisé).
  • select = ["ANN"] avec des erreurs d'annotations dans le projet et ceux-ci sont flaggués
  • select = ["ANN"] et ignore = ["ANN101"] avec des erreurs d'annotations dans le projet et toutes sont flaggués sauf `"ANN101"
  • select = ["ANN", "E", "F", "I", "N", "PD", "W"] avec de multiples erreurs dans le projet et celles-ci sont flaggués.

Bref, la méthode de faire un extend-select dans le fichier python-quality-checks.yml semble bonne pour étendre les règles d'un projet tout en imposant un ensemble de règles minimales à tous.

Pour ce qui est de ne pas échouer en cas d'absence du fichier: on utilise seulement le fichier pyproject.toml, celui-ci est nécessaire. Tout comme il est nécessaire que ruff soit inclus dans les dépendances du projets. Et c'était la même chose avec pycodestyle, celui-ci devait être présent dans les dépendances.
Quelqu'un qui ne veut pas utiliser de fichier pyproject.toml, ni ruff, devra rester sur la version 4.0.0. Je propose que l'on passe ici simplement à la version 5.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Ça me va.
Éventuellement, j'aimerais qu'on trouve un moyen de partager les configs et d'avoir l'option de les extend (pour désactiver des ruleset par exemple) plutôt que les copier-coller.
C'est ce qu'on fait en javascript et kotlin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ça fait un petit moment, je suis de retour sur cette question 😅
Je ne suis pas trop certain de comprendre comment tu veux que l'on se partage les configs. Est-ce que tu veux que l'on discute de ça ici ?

Copy link
Contributor

Choose a reason for hiding this comment

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

On peut en parler rapidement vendredi si tu veux. Je crée un event.

Copy link
Contributor

@kbeaulieu kbeaulieu left a comment

Choose a reason for hiding this comment

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

On veut ajouter l'option d'ajouter des ignore comme on parlait l'autre jour?

@AlexisGauthierAtKronos
Copy link
Contributor Author

On veut ajouter l'option d'ajouter des ignore comme on parlait l'autre jour?

Oui, je termine présentement les tests. Tout fonctionne comme prévu.

J'aurais aimé aussi inclure ici --line-length 120 mais il semble que ça ne puisse ensuite plus être overridé localement par le projet. Il faudra donc le définir dans chaque projet pour ne pas imposer à tous cette valeur. Un projet qui ne le spécifie pas aura le défaut 88.

Ainsi dans chaque projet on pourra définir quelque chose comme :

[tool.ruff]
ignore = ["ANN", "W"]  # ou toute autre ensemble de règles que l'on veut ignorer. On peut évidemment aussi omettre cette ligne pour utiliser toutes les règles spécifiées ici.
line-length = 120

@AlexisGauthierAtKronos AlexisGauthierAtKronos merged commit 0cad81f into main Jun 20, 2023
@AlexisGauthierAtKronos AlexisGauthierAtKronos deleted the P22-856 branch June 20, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants