👐🏿 🤽🏻 🧑🏽‍🤝‍🧑🏻 Pylint: un test détaillé de l'analyseur de code 🎅🏼 ❓ ♥️

Lorsque Luke a travaillé avec Flake8 et gardé un œil sur Pylint, il avait l'impression que 95% des erreurs générées par Pylint étaient fausses. D'autres développeurs avaient une expérience différente en interaction avec ces analyseurs, alors Luke a décidé d'examiner la situation en détail et d'étudier son travail sur 11 mille lignes de son code. En outre, il a salué les avantages de Pylint, le considérant comme un ajout à Flake8.

Luke ( Luke Plant ) - l'un des développeurs britanniques, sur l'article dont nous avons récemment rencontré l'analyse des analyseurs de code populaires. Les linters étudient le code, vous aident à trouver des bogues et le rendent stylistiquement cohérent avec les normes et le code que les développeurs de votre équipe écrivent. Les plus courants sont Pylint et Flake8. Nous sommes dans Leader-IDNous les utilisons également, nous avons donc traduit avec plaisir son article. Nous espérons que cela enrichira votre expérience avec ces outils.

Paramètres initiaux et base de test

Pour cette expérience, j'ai pris une partie du code d'un de mes projets et lancé Pylint avec les paramètres de base. Il a ensuite essayé d'analyser le résultat: quels avertissements étaient utiles et lesquels étaient faux.

Un peu d'aide sur le projet dont le code a été tiré:

Une application régulière écrite en Django (c'est-à-dire à l'intérieur du même Python). Django a ses propres particularités, et, en tant que framework, il a ses limites, mais vous permet d'écrire du code Python normal. D'autres bibliothèques qui utilisent des modèles (fonctions de rappel ou modèles de conception de méthode de modèle) ont également certains de ses inconvénients en tant que cadre.
Se compose de 22 000 lignes de code. Environ 11 000 lignes ont transité par Pylint (9 000 si des lacunes ont été omises). Cette partie du projet consistait principalement en vues et code de test.
Pour analyser le code de ce projet, j'ai déjà utilisé Flake8, ayant traité toutes les erreurs reçues. Le but de cette expérience était d'évaluer les avantages de Pylint en complément de Flake8.
Le projet a une bonne couverture de test du code, mais comme je suis son seul auteur, je n'ai pas eu l'occasion de recourir à la revue par les pairs.

J'espère que cette analyse sera utile à d'autres développeurs pour décider d'utiliser Pylint dans la pile pour vérifier la qualité du code. Je suppose que si vous utilisez déjà quelque chose comme Pylint, vous l'utiliserez systématiquement pour le contrôle de qualité nécessaire du code afin de minimiser les problèmes.

Ainsi, Pylint a émis 1650 réclamations sur mon code.

Ci-dessous, j'ai regroupé tous les commentaires de l'analyseur par type et leur ai donné mes commentaires. Si vous avez besoin d'une description détaillée de ces messages, consultez la section appropriée de la liste des fonctions Pylint.

Bugs

Pylint a trouvé exactement un bogue dans mon code. Je considère un bogue comme une erreur qui se produit ou peut potentiellement se produire pendant le fonctionnement du programme. Dans ce cas, j'ai utilisé des exceptions - broad-except.c'est -à -dire except Exception, pas seulement sauf, ce que Flake8 attrape. Cela entraînerait un comportement d'exécution à quelques exceptions près. Si cette erreur est apparue au moment de l'exécution (pas le fait qu'elle s'affiche), un comportement de code incorrect n'aurait pas de conséquences graves, cependant ...

Total: 1

Utile

En plus de cette erreur, Pylint en a trouvé quelques autres que j'ai classés comme utiles. Le code n'est pas tombé d'eux, mais il pourrait y avoir des problèmes lors de la refactorisation et, en principe, des erreurs à l'avenir lors de l'extension de la liste des fonctionnalités et de la prise en charge du code.

Sept d'entre eux étaient too-many-locals/ too-many-branches/ too-many-local-variables. Ils appartenaient à trois parties de mon code qui étaient mal structurées. Ce serait bien de penser à la structure, et je suis sûr que je pourrais faire mieux.

Autres erreurs:

unused-argument× 3 - l'un d'eux était vraiment un montant, et le code s'exécutait correctement de manière aléatoire. Les deux autres arguments inutiles et inutilisés entraîneraient des problèmes à l'avenir si je les utilisais.
redefined-builtin × 2
dangerous-default-value × 2 - pas de bugs, car je n'ai jamais utilisé les valeurs par défaut, mais ce serait bien de corriger ça juste au cas où.
stop-iteration-return × 1 - ici j'ai appris quelque chose de nouveau pour moi, je ne l'aurais jamais trouvé moi-même.
no-self-argument × 1

Total: 16

Modifications cosmétiques

Je ferais moins attention à ces choses. Ils sont soit insignifiants, soit improbables. En revanche, leur correction ne sera pas superflue. Certains d'entre eux sont stylistiques controversés. J'ai parlé de bancs similaires dans d'autres sections, mais ceux qui sont répertoriés ici conviennent également à ce contexte. En utilisant Pylint régulièrement, je corrigerais ces «défauts», mais dans la plupart des cas, je ne m'en inquiéterais pas.

invalid-name× 192

Il s'agissait essentiellement de noms de variables à une seule lettre. Utilisé dans les contextes où cela ne faisait pas peur, par exemple:

Beaucoup étaient dans le code de test:

len-as-condition × 20
useless-object-inheritance × 16 (héritage Python 2)
no-else-return × 11
no-else-raise × 1
bad-continuation × 6
redefined-builtin × 4
inconsistent-return-statements × 1
consider-using-set-comprehension × 1
chained-comparison × 1

TOTAL: 252

Inutile

C'est à ce moment-là que j'ai eu raison d'écrire le code comme je l'ai écrit, même si à certains endroits cela semble inhabituel. Et, à mon avis, il vaut mieux le laisser sous cette forme, même si certains ne seront pas d'accord ou préféreront un style d'écriture de code différent qui pourrait potentiellement éviter des problèmes.

too-many-ancestors × 76

Étaient dans le code de test où j'ai utilisé de nombreuses classes d' impuretés pour mettre des utilitaires ou des talons.

unused-variable × 43

Il a été trouvé presque tout le temps dans le code de test, où j'ai battu le record:

... et n'a utilisé aucun des éléments. Il existe plusieurs façons d'empêcher Pylint de signaler des erreurs (par exemple, donner des noms unused). Mais si vous le laissez sous la forme dans laquelle j'ai écrit, il sera lisible et les gens (y compris moi-même) pourront le comprendre et le soutenir.

invalid-name × 26

Ce sont des cas où j'ai attribué des noms appropriés dans le contexte, mais ceux qui ne respectaient pas les normes de dénomination Pylint. Par exemple, db (il s'agit d'une abréviation courante pour base de données) et d'autres noms non standard, qui, à mon avis, étaient plus compréhensibles. Mais vous n'êtes peut-être pas d'accord avec moi.

redefined-outer-name × 16

Parfois, un nom de variable est orthographié correctement pour les contextes internes et externes. Et vous n'aurez jamais à utiliser un nom externe à partir d'un contexte interne.

too-few-public-methods × 14

Les exemples incluent des classes avec des données créées à l'aide d' attrs , où il n'y a pas de méthodes publiques, et une classe qui implémente l'interface du dictionnaire, mais qui est nécessaire pour s'assurer que la méthode fonctionne correctement__getitem__

no-self-use × 12

Ils sont apparus dans le code de test, où j'ai intentionnellement ajouté des méthodes à la classe de base sans paramètre self, car de cette façon, il était plus pratique de les importer et de les rendre disponibles pour le cas de test. Certains d'entre eux ont même été intégrés dans des fonctions distinctes.

attribute-defined-outside-init × 10

Dans ces cas, il y avait de bonnes raisons d'écrire le code tel quel. Fondamentalement, ces erreurs se sont produites dans le code de test.

too-many-locals× 6, too-many-return-statements× 6, too-many-branches× 2, too-many-statements× 2

Oui, ces fonctionnalités étaient trop longues. Mais en les regardant, je n'ai pas vu de bons moyens de les nettoyer et de les améliorer. L'une des caractéristiques était simple, bien que longue. Il avait une structure très claire et n'était pas écrit de façon tordue, et tout moyen de le réduire auquel je pouvais penser inclurait des couches inutiles de fonctions inutiles ou gênantes.

arguments-differ × 6

Cela est principalement dû à l'utilisation de * args et ** kwargs dans la méthode surchargée, ce qui vous permet de vous protéger contre les changements dans la signature des méthodes des bibliothèques tierces (mais dans certains cas, ce message peut indiquer de vrais bugs).

ungrouped-imports × 4

J'utilise déjà isort pour importer

fixme × 4

Oui, il y a plusieurs choses qui doivent être corrigées, mais pour le moment je ne veux pas les corriger.

duplicate-code × 3

Parfois, vous utilisez une petite quantité de code passe-partout, ce qui est simplement nécessaire, et lorsqu'il n'y a pas beaucoup de logique réelle dans le corps de la fonction, cet avertissement vole.

broad-except × 2
abstract-method × 2
redefined-builtin × 2
too-many-lines × 1

J'ai essayé de comprendre de quelle manière naturelle briser ce module, mais je n'ai pas pu. C'est un exemple où vous pouvez voir que le linter est le mauvais outil. Si j'ai un module avec 980 lignes de code, et j'ajoute 30 autres, je franchis la limite de 1000 lignes, et les notifications du linter ne m'aideront pas ici. Si 980 lignes vont bien, alors pourquoi 1010 est-il mauvais? Je ne veux pas refactoriser ce module, mais je veux que le linter ne produise pas d'erreurs. La seule solution à ce moment que je vois est de faire en sorte que le linter soit silencieux, et cela contredit le but ultime.

pointless-statement × 1
expression-not-assigned × 1
cyclic-import × 1

Nous avons compris le cycle en déplaçant ses parties vers l'une des fonctions. Je ne pouvais pas trouver de meilleure façon de structurer le code en fonction des restrictions.

unused-import × 1

J'ai déjà ajouté # NOQAlors de l'utilisation de Flake8 afin que cette erreur ne s'affiche pas.

too-many-public-methods × 1

Si dans ma classe de test il y a 35 tests au lieu de 20 réglementés, est-ce vraiment un problème?

too-many-arguments × 1

Total: 243

Impossible de corriger

Cette catégorie reflète les erreurs que je ne peux pas corriger même si je le souhaitais, en raison de restrictions externes, telles que la nécessité de renvoyer ou de transmettre des classes à une bibliothèque ou à un framework tiers qui doivent satisfaire certaines exigences.

unused-argument × 21
invalid-name × 13
protected-access × 3

Accès inclus aux "objets internes documentés", comme sys._getframedans stdlib et Django Model._meta.

too-few-public-methods × 3
too-many-arguments × 2
wrong-import-position × 2
attribute-defined-outside-init × 1
too-many-ancestors × 1

Total: 46

Faux messages

Des choses dans lesquelles Pylint a clairement tort. Dans ce cas, ce ne sont pas des erreurs Pylint: le fait est que Python est dynamique, et Pylint essaie de découvrir des choses qui ne peuvent pas être faites parfaitement ou de manière fiable.

no-member × 395

Associé à plusieurs classes de base: de Django et celles que j'ai créées moi-même. Pylint n'a pas pu détecter la variable en raison du dynamisme / métaprogrammation.

De nombreuses erreurs se sont produites en raison de la structure du code de test (j'ai utilisé un modèle de django-functest, qui dans certains cas pourrait être corrigé en ajoutant des classes de base supplémentaires en utilisant les méthodes "abstraites" qui appellent NotImplementedError) ou, éventuellement, en renommant de nombreuses classes de test (I Je ne l'ai pas fait, car dans certains cas, ce serait déroutant).

invalid-name × 52

Le problème est principalement dû au fait que Pylint a appliqué la règle de constante PEP8 , considérant que chaque nom de niveau supérieur défini avec =est une «constante». Définir exactement ce que nous entendons par une constante est plus difficile qu'il n'y paraît, mais cela ne s'applique pas à certaines choses qui sont intrinsèquement constantes, par exemple les fonctions. De plus, la règle ne doit pas être appliquée à des méthodes moins connues de création de fonctions, par exemple:

Certains exemples sont discutables en raison du manque de définition de ce qu'est une constante. Par exemple, une instance d'une classe définie au niveau du module, qui peut ou non avoir un état mutable, doit-elle être considérée comme constante? Par exemple, dans cette expression:

no-self-use × 23

Pylint incorrectement déclaré La méthode pourrait être une fonction dans de nombreux cas où j'utilise l'héritage pour effectuer différentes implémentations, respectivement, je ne peux pas les convertir en fonctions.

protected-access × 5

Pylint a incorrectement évalué qui était le «propriétaire» (le fragment de code actuel crée l'attribut protégé de l'objet et l'utilise localement, mais Pylint ne le voit pas).

no-name-in-module × 1
import-error × 1
pointless-statement × 1

Cette déclaration a effectivement produit le résultat:

Je l'ai utilisé pour provoquer intentionnellement une erreur inhabituelle qui ne sera probablement pas trouvée par les tests. Je ne blâme pas Pylint de ne pas le reconnaître ...

Total: 477

Total

Nous ne sommes pas encore arrivés, mais il est temps de regrouper nos résultats:

"Bon" - Blocs "Bogues" et "Utilitaires" - ici Pylint a certainement aidé: 17.
«Neutre» - «Changements cosmétiques» - un avantage mineur de Pylint, les erreurs ne causeront pas de dommages: 252.
"Mauvais" - "Inutile", "Incapable de corriger", "Imprécisions" - où Pylint veut des changements dans le code, où ce n'est pas nécessaire. Y compris lorsque les modifications ne peuvent pas être apportées en raison de dépendances externes ou lorsque Pylint a simplement incorrectement analysé le code: 766.

Le rapport du bon au mauvais est très faible. Si Pylint était mon collègue aidant à la révision du code, je le supplierais de partir.

Pour supprimer les fausses notifications, vous pouvez noyer la classe d'erreur sortante (ce qui augmentera alors l'inutilité de l'utilisation de Pylint) ou ajouter des commentaires spéciaux au code. Je ne veux pas faire ce dernier, car:

Ça prend du temps!
Je n'aime pas empiler les commentaires qui existent pour faire taire le linter.

J'ajouterai volontiers ces pseudo-commentaires quand il y aura un avantage certain du linter. De plus, je suis soucieux des commentaires, donc ma coloration syntaxique les affiche de manière vivante: comme recommandé dans cet article . Cependant, à certains endroits, j'ai déjà ajouté # commentaires NOQAà Muffle Flake8, mais avec eux pour une section, vous ne pouvez ajouter que cinq codes d'erreur.

Docstrings

Les problèmes restants que Pylint a découverts manquaient de lignes de documentation. Je les mets dans une catégorie distincte car:

Ils sont très controversés, et vous pouvez avoir une politique complètement différente concernant de telles choses.
Je n'ai pas eu le temps de les analyser tous.

Au total, Pylint a découvert 620 dockstrings manquants (dans les modules, les fonctions, les méthodes de classe). Mais dans de nombreux cas, j'avais raison. Par exemple:

Quand tout est clair d'après le nom. Par exemple:
Quand une docstring est déjà définie - par exemple, si j'implémente l'interface du routeur de base de données de Django . Ajouter votre propre ligne dans ce cas peut être dangereux.

Dans d'autres cas, ces lignes de descriptions ne nuiraient pas à mon code. Dans environ 15 à 30% des cas trouvés par Pylint, j'aurais pensé: "Oui, je dois ajouter une docstring ici, merci Pylint pour le rappel."

En général, je n'aime pas les outils qui permettent d'ajouter des cordons d'ancrage partout et partout, car je pense que dans ce cas, ils se révéleront mauvais. Il vaut mieux ne pas les écrire du tout. Une situation similaire avec de mauvais commentaires:

les lire est une perte de temps: ils ne disposent pas d'informations complémentaires ou contiennent des informations incorrectes,
ils aident à surligner inconsciemment les docstrings dans le texte, car ils contiennent des informations utiles (si vous les écrivez dans le cas).

Les avertissements concernant les lignes de documentation manquantes sont ennuyeux: pour les supprimer, vous devez ajouter des commentaires séparément, ce qui prend environ le même temps que l'ajout de l'ancrage lui-même. De plus, tout cela crée du bruit visuel. En conséquence, vous obtiendrez des lignes de documentation qui ne sont ni nécessaires ni utiles.

Conclusion

Je crois que mes hypothèses initiales sur l'inutilité de Pylint se sont avérées correctes (dans le contexte de la base de code pour laquelle j'utilise Flake8). Pour que je puisse l'utiliser, l'indicateur reflétant le nombre de faux positifs doit être inférieur.

En plus de créer du bruit visuel, un délai supplémentaire est nécessaire pour trier ou filtrer les fausses notifications, et je ne voudrais pas ajouter quoi que ce soit au projet. Les développeurs juniors seront plus humbles pour apporter des modifications afin de supprimer les commentaires de Pylint. Mais cela les conduira à casser le code de travail, à ne pas réaliser que Pylint est erroné, ou au fait que vous aurez donc beaucoup de refactoring pour que Pylint puisse comprendre votre code correctement.

Si vous utilisez Pylint dans un projet depuis le tout début ou dans un projet où il n'y a pas de dépendances externes, je pense que vous aurez une opinion différente et le nombre de fausses notifications sera moindre. Cependant, cela peut entraîner des coûts de temps supplémentaires.

Une autre approche consiste à utiliser Pylint pour un nombre limité de types d'erreurs. Cependant, il n'y en avait que quelques-unes, dont les réponses se sont avérées correctes ou extrêmement rarement fausses (en termes relatifs et absolus). Parmi ceux - ci sont: dangerous-default-value, stop-iteration-return, broad-exception, useless-object-inheritance.

En tout cas, j'espère que cet article vous a aidé lorsque vous envisagez d'utiliser Pylint ou en cas de litige avec des collègues.

Pylint: un test détaillé de l'analyseur de code