tournesol icon indicating copy to clipboard operation
tournesol copied to clipboard

Pylint upgrade and 1st wave of warnings corrections (#448)

Open glerzing opened this issue 2 years ago • 0 comments

Here is a first shot at pylint warnings, after upgrading the pylint version. I'm waiting for some feedback to know if I should continue.

Do we want to have a zero-warnings strategy and include it in the continuous integration? (is it productive? pylint warnings can be more tricky to solve than flake8 warnings) If yes, which warnings to disable?

On our previous configuration file, .pylintrc, we made no customization. The .pylintrc generated with the new Pylint version looks very different, but the only important difference is about the "disable" variable (indicating ignored warning types) => fewer warning types are ignored by default. Here is what I modified from the default pylint config:

  • I added serializers to the list of ignored directories names (I estimated that the warnings there were not really interesting to "solve" ; the only 2 types of warnings there were missing-class-docstring and abstract-method)
  • I disabled the warning types:
    • missing-module-docstring (we didn't get the habit to make module docstrings)
    • too-many-ancestors (sometimes when inheriting from Django classes, the inheritance level can quickly get high and it's hard to find a workaround anyway)
    • too-few-public-methods (some classes are used just to store data and don't have methods, and it's not so much an issue, I still fixed it using the decorator dataclass when it was relevant)
    • use-maxsplit-arg (only appeared once: published_date.split("T")[0] => it's just as easy to solve the warning, but I don't find it very meaningful either as a performance improvement)

Warnings are by default disabled on the repertories called "tests". I did not add the machine learning (ml) to the directories checked by Pylint in lint.sh. Like with the serializers, most of the warnings there don't really look interesting.

bandit was also upgraded, and gave a single warning due to the use of random.choices. I deactivated the warning, because although there are alternatives to random.choice (e.g. in the library secrets), there doesn't seem to be an alternative to random.choices, with a "s".

Some other checkers that we could chose to disable:

  • missing-class-docstring
  • import-outside-toplevel (hard to really solve)
  • consider-using-f-string (I suspect it was usually done on purpose, not using f-strings)
  • broad-except (broadly catching exceptions might be legitimate sometimes)
  • abstract-method (usually not so useful)
  • logging-fstring-interpolation (apparently, this is mostly useful for log aggregators, but I don't think we are using one)

Warnings can be disabled per file or per line, for example with "# pylint: disable=missing-class-docstring, too-many-arguments"

If you don't like some docstrings or other corrections, feel free to modify it directly in my branch. And of course, you can disagree with my decisions for disabled warning types and for disabling analysis on serializers, no problem with that.

Here are the current logs: logs_pylint.txt

glerzing avatar Aug 10 '22 21:08 glerzing