⏮️ 🍇 🧝🏾 Pylint: a detailed test of the code analyzer 🤾 ⚽️ 👨🏽‍💼

When Luke worked with Flake8 and kept an eye on Pylint, he had the impression that 95% of the errors generated by Pylint were false. Other developers had different experience interacting with these analyzers, so Luke decided to look into the situation in detail and study his work on 11 thousand lines of his code. In addition, he praised the benefits of Pylint, viewing it as an addition to Flake8.

Luke ( Luke Plant ) - one of the British developers, on whose article with the analysis of popular code analyzers we recently came across. Linters study the code, help you find bugs, and make it stylistically consistent with the standards and code that the developers on your team write. The most common ones are Pylint and Flake8. We are in Leader-IDWe also use them, so we happily translated his article. We hope that it will enrich your experience with these tools.

Initial settings and test base

For this experiment, I took part of the code from one of my projects and launched Pylint with basic settings. Then he tried to analyze the result: which warnings were useful and which were false.

A little help about the project from which the code was taken:

A regular application written in Django (i.e. inside the same Python). Django has its own peculiarities, and, as a framework, it has its limitations, but allows you to write normal Python code. Other libraries that use templates (callback functions or Template Method design templates) also have some of its drawbacks as a framework.
Consists of 22,000 lines of code. Approximately 11,000 lines passed through Pylint (9,000 if omitted gaps). This part of the project consisted mainly of views and test code.
To analyze the code for this project, I already used Flake8, having processed all the errors received. The point of this experiment was to evaluate the benefits of Pylint as an addition to Flake8.
The project has a good test coverage of the code, but since I am its sole author, I did not have the opportunity to use peer review.

I hope that this analysis will be useful to other developers to decide on using Pylint in the stack to check the quality of the code. I assume that if you are already using something like Pylint, you will use it systematically for the necessary quality control of the code in order to minimize problems.

So, Pylint has issued 1650 claims to my code.

Below, I grouped all the analyzer comments by type and gave my comments to them. If you need a detailed description of these messages, take a look at the appropriate section of the Pylint function list.

Bugs

Pylint found exactly one bug in my code. I consider a bug as an error that occurs or may potentially arise during the operation of the program. In this case, I used exceptions - broad-except.That is except Exception, not just except, which Flake8 catches. This would result in runtime behavior with some exceptions. If this error ever popped up at run time (not the fact that it pops up), then incorrect code behavior would not have serious consequences, though ...

Total: 1

Useful

In addition to that error, Pylint found a few more that I categorized as useful. The code didn’t fall from them, but there could be problems during refactoring, and in principle, errors in the future when expanding the list of features and supporting the code.

Seven of them were too-many-locals/ too-many-branches/ too-many-local-variables. They belonged to three parts of my code that were poorly structured. It would be nice to think about the structure, and I'm sure I could do better.

Other errors:

unused-argument× 3 - one of them was really a jamb, and the code executed correctly randomly. The other two unnecessary and unused arguments would lead to problems in the future if I used them.
redefined-builtin × 2
dangerous-default-value × 2 - not bugs, because I never used the default values, but it would be nice to fix this just in case.
stop-iteration-return × 1 - here I learned something new for myself, I would never have found it myself.
no-self-argument × 1

Total: 16

Cosmetic edits

I would pay less attention to these things. They are either insignificant or unlikely. On the other hand, their correction will not be superfluous. Some of them are controversial stylistic. I talked about some similar jambs in other sections, but those that are listed here are also suitable for this context. Using Pylint regularly, I would correct these “flaws”, but in most cases I would not worry about them.

invalid-name× 192

These were basically single letter variable names. Used in those contexts where it wasn’t scary, for example:

Many were in the test code:

len-as-condition × 20
useless-object-inheritance × 16 (Python 2 legacy)
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

Useless

This is when I had reason to write the code the way I wrote it, even if in some places it seems unusual. And, in my opinion, it is better to leave it in this form, although some will not agree or prefer a different style of writing code that could potentially avoid problems.

too-many-ancestors × 76

Were in the test code where I used many classes of impurities to put utilities or stubs.

unused-variable × 43

It was found almost all the time in the test code, where I broke the record:

... and did not use any of the elements. There are several ways to prevent Pylint from reporting errors (for example, give names unused). But if you leave it in the form in which I wrote, it will be readable, and people (including myself) will be able to understand and support it.

invalid-name × 26

These were cases where I assigned appropriate names in the context, but those that did not meet the Pylint naming standards. For example, db (this is a common abbreviation for database) and some other non-standard names, which, in my opinion, were more understandable. But you may not agree with me.

redefined-outer-name × 16

Sometimes a variable name is spelled correctly for both internal and external contexts. And you will never have to use an external name from an internal context.

too-few-public-methods × 14

Examples include classes with data created using attrs , where there are no public methods, and a class that implements the dictionary interface, but which is needed to ensure the method works correctly__getitem__

no-self-use × 12

They appeared in the test code, where I intentionally added methods to the base class without a parameter self, because in this way it was more convenient to import them and make them available for the test case. Some of them even wrapped in separate functions.

attribute-defined-outside-init × 10

In these cases, there were good reasons to write the code as it is. Basically, these errors occurred in the test code.

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

Yes, these features were too long. But looking at them, I did not see any good ways to clean and improve them. One of the features was simple, albeit long. It had a very clear structure and was not crookedly written, and any ways of reducing it that I could think of would include useless layers of unnecessary or inconvenient functions.

arguments-differ × 6

This is mainly due to the use of * args and ** kwargs in an overridden method, which allows you to protect yourself from changes in the signature of methods from third-party libraries (but in some cases this message may indicate real bugs).

ungrouped-imports × 4

I already use isort to import

fixme × 4

Yes, there are several things that need to be fixed, but right now I don’t want to fix them.

duplicate-code × 3

Sometimes you use a small amount of boilerplate code, which is simply necessary, and when there is not much real logic in the function body, this warning flies.

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

I tried to figure out in what natural way to break this module, but could not. This is one example where you can see that the linter is the wrong tool. If I have a module with 980 lines of code, and I add another 30, I cross the limit of 1000 lines, and notifications from the linter will not help me here. If 980 lines is fine, then why is 1010 bad? I do not want to refactor this module, but I want the linter to not produce errors. The only solution at this moment I see is to make somehow so that the linter is silent, and this contradicts the ultimate goal.

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

We figured out the cycle by moving its parts to one of the functions. I could not find a better way to structure the code based on restrictions.

unused-import × 1

I already added # NOQAwhen using Flake8 so that this error does not pop up.

too-many-public-methods × 1

If in my test class there are 35 tests instead of 20 regulated ones, is this really a problem?

too-many-arguments × 1

Total: 243

Unable to fix

This category reflects errors that I can’t fix even if I wanted to, due to external restrictions, such as the need to return or pass classes to a third-party library or framework that must satisfy certain requirements.

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

Included access to "documented internal objects", such as sys._getframein stdlib and 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

False messages

Things in which Pylint is clearly wrong. In this case, these are not Pylint errors: the fact is that Python is dynamic, and Pylint is trying to discover things that cannot be done perfectly or reliably.

no-member × 395

Associated with several base classes: from Django and those that I created myself. Pylint could not detect the variable due to dynamism / metaprogramming.

Many errors occurred due to the structure of the test code (I used a template from django-functest, which in some cases could be corrected by adding additional base classes using the "abstract" methods that call NotImplementedError) or, possibly, renaming many test classes (I I didn’t do it, because in some cases it would be confusing).

invalid-name × 52

The problem arose mainly because Pylint applied the PEP8 constant rule , believing that every top-level name defined with =is a “constant”. Defining exactly what we mean by a constant is more difficult than it seems, but this does not apply to some things that are inherently constants, for example, functions. Also, the rule should not be applied to less familiar ways of creating functions, for example:

Some examples are debatable due to the lack of definition of what a constant is. For example, should an instance of a class defined at the module level, which may or may not have a mutable state, be considered constant? For example, in this expression:

no-self-use × 23

Pylint incorrectly stated Method could be a function for many cases where I use inheritance to perform various implementations, respectively, I can not convert them to functions.

protected-access × 5

Pylint incorrectly assessed who was the “owner” (the current code fragment creates the protected attribute of the object and uses it locally, but Pylint does not see this).

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

This statement actually produced the result:

I used this to intentionally cause an unusual error that is unlikely to be found by tests. I don’t blame Pylint for not recognizing it ...

Total: 477

Subtotal

We are not yet at the finish line, but it's time to group our results:

“Good” - “Bugs” and “Utilities” blocks - here Pylint definitely helped: 17.
“Neutral” - “Cosmetic changes” - a minor benefit from Pylint, errors will not cause damage: 252.
“Bad” - “Useless”, “Unable to fix”, “Inaccuracies” - where Pylint wants changes in the code, where it is not required. Including where edits cannot be made due to external dependencies or where Pylint simply incorrectly analyzed the code: 766.

The ratio of good to bad is very small. If Pylint was my colleague helping code review, I would beg him to leave.

To remove false notifications, you can drown out the outgoing error class (which will then increase the uselessness of using Pylint) or add special comments to the code. I don’t want to do the latter, because:

It takes time!
I don't like the piling up of comments that exist to silence the linter.

I will gladly add these pseudo-comments when there will be a definite plus from the linter. In addition, I am anxious about comments, so my syntax highlighting displays them vividly: as recommended in this article . However, in some places I have already added # comments NOQAto muffle Flake8, but with them for one section you can add only five error codes.

Docstrings

The remaining problems that Pylint discovered were missing documentation lines. I put them in a separate category because:

They are very controversial, and you may have a completely different policy regarding such things.
I did not have time to analyze all of them.

In total, Pylint discovered 620 missing dockstrings (in modules, functions, class methods). But in many cases, I was right. For instance:

When everything is clear from the name. For instance:
When a docstring is already defined - for example, if I implement the Django's database router interface . Adding your own line in this case can be dangerous.

In other cases, these lines of descriptions would not hurt my code. In about 15–30% of the cases found by Pylint, I would have thought, “Yes, I need to add a docstring here, thanks Pylint for the reminder.”

In general, I don’t like the tools that make adding dockstrings everywhere and everywhere, because I think that in this case they will turn out bad. It’s better not to write them at all. A similar situation with bad comments:

reading them is a waste of time: they do not have additional information or they contain incorrect information,
they help subconsciously highlight docstrings in the text, because they contain useful information (if you write them in the case).

Warnings about missing documentation lines are annoying: to remove them, you need to add comments separately, which takes about the same amount of time as adding the docking itself. Plus it all creates visual noise. As a result, you will get lines of documentation that are not necessary or useful.

Conclusion

I believe that my initial assumptions about the uselessness of Pylint turned out to be correct (in the context of the code base for which I use Flake8). In order for me to use it, the indicator reflecting the number of false positives should be lower.

In addition to creating visual noise, additional time is required to sort out or filter out false notifications, and I would not want to add any of this to the project. Junior developers will be more humble to make edits to remove Pylint's comments. But this will lead to them breaking the working code, not realizing that Pylint is wrong, or to the fact that as a result you will have a lot of refactoring so that Pylint can understand your code correctly.

If you use Pylint in a project from the very beginning or in a project where there are no external dependencies, I think you will have a different opinion and the number of false notifications will be less. However, this can lead to additional time costs.

Another approach is to use Pylint for a limited number of kinds of errors. However, there were only a few, the responses on which turned out to be correct or extremely rarely false (in relative and absolute terms). Among them are: dangerous-default-value, stop-iteration-return, broad-exception, useless-object-inheritance.

In any case, I hope this article helped you when considering whether to use Pylint or in a dispute with colleagues.

Pylint: a detailed test of the code analyzer