Pylint usage

[berland]

Pylint is enforced in the CI, which means that any pylint recommendation the contributor is unable to solve, is added as a pylint exception.

Adding pylint exceptions make it harder to assess code quality, and suppresses the incentive that a sub-10 pylint score should give.

Currently, the in-code pylint exceptions are:

(3.8) ✔ ~/projects/webviz-subsurface [master|✔] 
15:13 $ grep --include="*.py" "pylint: disable" -r 2>/dev/null | cut -f2 -d\# | cut -f2 -d= | grep -v PyCQA | tr -d , | fmt -w1 | sort | uniq -c | sort -nr
     41 too-many-arguments
     34 too-many-locals
     24 no-name-in-module
     13 too-many-statements
     11 too-many-branches
     10 too-few-public-methods
      5 unused-argument
      5 too-many-instance-attributes
      5 no-member
      3 too-many-nested-blocks
  [snip]

A suggestion is to use a different pylint configuration file in the CI that allows all (?) these, and then remove the in-code exceptions unless they can be argued persuasively for (for example if fixing the pylint recommendation really makes the code worse).

webviz-subsurface has a top score at https://lgtm.com/projects/g/equinor/webviz-subsurface/context:python but this is arguably a fake 😉

[anders-kiaer]

Thanks for the suggestion @berland

A suggestion is to use a different pylint configuration file in the CI that allows all (?) these, and then remove the in-code exceptions unless they can be argued persuasively for (for example if fixing the pylint recommendation really makes the code worse).

I don't think I completely understand what we would we achieve compared to today with having two different pylint configurations :thinking: Different contributors have added exceptions where they have not had time or necessary skills to improve a particular code block, arguably not perfect, but with explicit pylint: disable= in the relevant areas it is easier for them and others to see which part of the code needs improvement later compared to globally relaxing pylint?

Reviewers in webviz-subsurface I think rarely run pylint manually locally on incoming PRs from others, since it is enforced automatically by the CI. Seeing a pylint: disable= in the code and letting that through (or not) is a compromise between the reviewer and the contributor.

webviz-subsurface has a top score at https://lgtm.com/projects/g/equinor/webviz-subsurface/context:python but this is arguably a fake :wink:

Is it? As far as I know LGTM.com don't use any of the standard tools (pylint, flake8, bandit, mypy...) in their analysis, it is only based LGTM's CodeQL queries. :slightly_smiling_face:

[berland]

Thanks for the suggestion @berland

A suggestion is to use a different pylint configuration file in the CI that allows all (?) these, and then remove the in-code exceptions unless they can be argued persuasively for (for example if fixing the pylint recommendation really makes the code worse).

I don't think I completely understand what we would we achieve compared to today with having two different pylint configurations 🤔 Different contributors have added exceptions where they have not had time or necessary skills to improve a particular code block, arguably not perfect, but with explicit pylint: disable= in the relevant areas it is easier for them and others to see which part of the code needs improvement later compared to globally relaxing pylint?

Reviewers in webviz-subsurface I think rarely run pylint manually locally on incoming PRs from others, since it is enforced automatically by the CI. Seeing a pylint: disable= in the code and letting that through (or not) is a compromise between the reviewer and the contributor.

webviz-subsurface has a top score at https://lgtm.com/projects/g/equinor/webviz-subsurface/context:python but this is arguably a fake 😉

Is it? As far as I know LGTM.com don't use any of the standard tools (pylint, flake8, bandit, mypy...) in their analysis, it is only based LGTM's CodeQL queries. 🙂

[berland]

The problem is that the pylint: disable statements in the webviz source do not differentiate between false positives and procrastination. They should be used for the first, but not for the second. I am ok with procastrination, but it should not be disguised as false positives, and it should not give "noise" in the source files.

Pylint has false positives in most code bases, and flagging in the source them has value.

Maybe pylint-ignore could be a solution.

LGTM might ignore the disable-statements, I am not sure, but the pylint score at 10 is at least not correct :)

[anders-kiaer]

The distinction between false positives and code blocks with improvement potential is in most cases quite clear already in my opinion(as the latter typically is of the format too-many/few-*), but we could perhaps improve it further★.

I'm not sure if I would call the pylint disable statements for the latter group noise in the code. The former group is a false positive (and necessary), the latter clearly states to any code readers that there are room for improvements in that code block if they want to give it a try (they can search for pylint: disable= directly in GitHub UI as well).

Letting the CI having a relaxed pylint configuration has its own risks:

• PR author or reviewer forgetting to run pylint with more strict pylint config locally a last time before merge.
• We have two configs instead of one for pylint which could be confusing for new contributors.

LGTM might ignore the disable-statements, I am not sure, but the pylint score at 10 is at least not correct :)

It doesn't ignore them, it doesn't use pylint at all 😉 The pylint score being at 10 is not "correct", but it is also not used for anything (except for CI not allowing a score less than 10 of new incoming PRs, making the PR author aware of code improvement areas).

---

★ We could perhaps implement a contribution guideline that all pylint disable= not being of type too-many/few.* should be linked to a pylint issue in the comment (or at least have an explanation of why the disable is needed in the code). We have done that on a couple of occasions already, but not routinely. I think that could be an improvement in the contribution workflow. Difficult perhaps to automatically CI check, but we could try to socially implement it. This should then be accompanied (in the same PR) adding this requirement for all pylint: disable not of type too-many/few.*. Or while first at it, maybe all disable could have a sentence accompanied with it?

As a side note, thanks to useless-suppression in https://github.com/equinor/webviz-subsurface/blob/259f678650e7d8bf84914d6f7e58914d4b82b090/.pylintrc#L4 pylint (and therefore CI) will complain if a previous false positive (or any other disable) is not needed anymore, removing at least one use case for a clear distinction.

[berland]

Fair enough. I think pylint-ignore tries to balance this in a good way, but todays attemts in using it for ecl2df and pyscal has not been succesful enough to add it to those CIs.