Remove globally disabled `pylint` messages #522

marvinpfoertner · 2021-08-16T14:17:41Z

In a Nutshell

This PR removes most globally disabled pylint messages.

Detailed Description

When we introduced pylint for linting, we decided to disable different messages on a package level via tox.ini in order to be able to use pylint in our CI pipeline while keeping the flexibility to improve one package at a time. However, we noticed that this way we lacked a "global" run linting the package as a whole. Consequently, we decided to disable the union of all disabled messages to the global pylint configuration to be able to run pylint locally.
Unfortunately, this caused the CI to accept code that will not be accepted by our long-term pylint config, even if the message was removed from the per-package disable list.
This PR

removes all globally disabled pylint messages from pyproject.toml except the ones which we decided to keep long-term,
disables these messages again on a per-package basis in tox.ini,
reformats and documents the pylint configuration in pyproject.toml,
adds an additional "global" pylint pass to tox.ini, and
refactors the pylint_check.py script from the CI to ensure that the messages disabled in the global linting pass are exactly the union of the messages disabled in the per-module linting passes.

Related Issues

Closes #469.

codecov · 2021-08-16T14:22:56Z

Codecov Report

Merging #522 (b3a14f5) into main (2dadf97) will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main     #522   +/-   ##
=======================================
  Coverage   87.54%   87.54%           
=======================================
  Files         151      151           
  Lines        5943     5943           
  Branches      771      771           
=======================================
  Hits         5203     5203           
  Misses        507      507           
  Partials      233      233

Impacted Files	Coverage Δ
src/probnum/__init__.py	`84.61% <ø> (ø)`
src/probnum/_config.py	`100.00% <ø> (ø)`
src/probnum/kernels/_kernel.py	`84.31% <ø> (ø)`
src/probnum/linops/_scaling.py	`81.81% <0.00%> (ø)`
src/probnum/randvars/_normal.py	`81.69% <ø> (ø)`
...probnum/problems/zoo/linalg/_suitesparse_matrix.py	`83.95% <100.00%> (ø)`

pnkraemer

Thanks for taking care of this! I left a bunch of comments, all of which are mostly out of curiosity. I think the changes are fine already. :)

pnkraemer · 2021-08-17T07:18:25Z

.github/workflows/pylint_check.py

    If you are not sure what exactly you are supposed to do, or if you think that this
-    message is wrong please feel free to ping @nathanaelbosch.
+    message is wrong please feel free to ping @marvinpfoertner or @nathanaelbosch.


Thanks for adding this. Purely out of curiosity: how well do you think these "ping XYZ" error messages will age?

Good point, I guess we can take this line out and have people ping any maintainer instead

I completely agree, people should open issues instead of pinging people (or ping them in the new opened issue) 👍

pnkraemer · 2021-08-17T07:19:06Z

pyproject.toml

+    # In functions/methods which are part of the public API, we often change the type of argument variables when normalizing inputs for internal processing,
+    # e.g. we might use `pn.utils.as_numpy_scalar` to cast a `float` into a `np.double` or a `np.ndarray` into a `RandomVariable` by using `pn.randvars.asrandvar`.
+    "redefined-variable-type",
+    # Many more `disable`s are defined in `./tox.ini` on a per-module basis!


[minor] would this be something that is removed once the last per-module is removed?

Yes. To be honest, I don't think this line is particularly well-placed here anymore. I'll remove it.

pnkraemer · 2021-08-17T07:20:07Z

pyproject.toml

+    # Extensions that might be of interest in the future:
+    # "pylint.extensions.check_elif",
+    # "pylint.extensions.docparams",


[minor] now that these lines are touched anyway, is this well-placed here? Would it be better as an issue/discussion instead?

I'll just enable the plugins and disable their messages per-package.
That way we can resolve them like any other message.

Is it already clear that we want those enabled? Depending on that I agree that you could just enable them, or we might need a discussion on what we actually want to enforce.

We certainly want docparams enabled and check_elif is also a good idea. I'll enable it for now and we can take it out if it causes trouble in the future. Is this fine @pnkraemer @nathanaelbosch?

For me this is fine. docparams will detect changes that one often forgets when refactoring (at least I do...). check_elif seems okay to me as well

tox.ini

nathanaelbosch · 2021-08-17T08:49:42Z

.github/workflows/pylint_check.py

    If you are not sure what exactly you are supposed to do, or if you think that this
-    message is wrong please feel free to ping @nathanaelbosch.
+    message is wrong please feel free to ping @marvinpfoertner or @nathanaelbosch.


I completely agree, people should open issues instead of pinging people (or ping them in the new opened issue) 👍

nathanaelbosch · 2021-08-17T08:51:38Z

pyproject.toml

+    # Extensions that might be of interest in the future:
+    # "pylint.extensions.check_elif",
+    # "pylint.extensions.docparams",


Is it already clear that we want those enabled? Depending on that I agree that you could just enable them, or we might need a discussion on what we actually want to enforce.

nathanaelbosch · 2021-08-17T08:55:31Z

tox.ini

+    pylint src/probnum/randvars --disable="too-many-arguments,too-many-locals,too-many-branches,too-few-public-methods,protected-access,unused-argument,no-else-return,duplicate-code,line-too-long,missing-function-docstring" --jobs=0
+    pylint src/probnum/utils --disable="no-else-return,line-too-long" --jobs=0
+    # Benchmark and Test Code Linting Pass
+    # pylint benchmarks --disable="unused-argument,attribute-defined-outside-init,missing-function-docstring" --jobs=0 # not a work in progress, but final


Is this comment supposed to indicate what the final pylint exceptions should be?

…fined_variable_type` message

marvinpfoertner · 2021-08-17T09:27:12Z

@pnkraemer @nathanaelbosch I incorporated your review comments.
If you're fine with the changes, please go ahead and merge.
I can't, since test coverage did not increase.

Bump pylint version

ce8fb63

marvinpfoertner requested review from JonathanWenger and pnkraemer August 16, 2021 14:17

marvinpfoertner self-assigned this Aug 16, 2021

marvinpfoertner added bug Something isn't working refactoring Refactoring of existing functionality testing and CI Unit tests, coverage and continuous integration labels Aug 16, 2021

marvinpfoertner added this to the Dagstuhl Workshop milestone Aug 16, 2021

marvinpfoertner added 18 commits August 16, 2021 16:58

Add custom message disables for diffeq

0d84694

Bump pylint version

4edf517

Clean up pyproject.toml

df51722

Fix indentation in tox.ini

92d8239

Add global pylint run to CI

19aa991

Add custom message disables for filtsmooth

4e970fe

Add custom message disables for kernels

2cc7e08

Add custom message disables for linalg

964636d

Add custom message disables for linops

d24929f

Add custom message disables for problems

ddb6ed6

Add custom message disables for quad

b1fa267

Add custom message disables for randprocs

58b5535

Add custom message disables for randvars

b675db7

Add custom message disables for utils

b630647

Add custom message disables for benchmarks

4fbc4e5

Add custom message disables for tests

4d4ac8a

Remove whitespace from pyproject toml

cc7c783

Refactor Nathanael's CI script to adapt to the new tox config

b62235c

marvinpfoertner force-pushed the remove-global-pylint-ignores branch from df4e80b to b62235c Compare August 16, 2021 15:09

marvinpfoertner requested a review from nathanaelbosch August 16, 2021 15:38

marvinpfoertner mentioned this pull request Aug 16, 2021

Refactor the code to comply with our desired pylint configuration #523

Open

11 tasks

Remove comment in tox.ini

8f9be57

pnkraemer approved these changes Aug 17, 2021

View reviewed changes

nathanaelbosch approved these changes Aug 17, 2021

View reviewed changes

marvinpfoertner added 4 commits August 17, 2021 11:08

Enable check_elif and docparams pylint extensions

f2bd227

Incorporate review comments

dbd0067

Disable redefined_variable_type exception and remove disabled `rede…

f4b2612

…fined_variable_type` message

Reorder pylint plugins based on pylint docs

b3a14f5

pnkraemer merged commit a55a5ae into probabilistic-numerics:main Aug 17, 2021

marvinpfoertner deleted the remove-global-pylint-ignores branch August 17, 2021 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove globally disabled `pylint` messages #522

Remove globally disabled `pylint` messages #522

marvinpfoertner commented Aug 16, 2021 •

edited

Loading

codecov bot commented Aug 16, 2021 •

edited

Loading

pnkraemer left a comment

pnkraemer Aug 17, 2021

marvinpfoertner Aug 17, 2021 •

edited

Loading

nathanaelbosch Aug 17, 2021

pnkraemer Aug 17, 2021

marvinpfoertner Aug 17, 2021

pnkraemer Aug 17, 2021

marvinpfoertner Aug 17, 2021

nathanaelbosch Aug 17, 2021

marvinpfoertner Aug 17, 2021

pnkraemer Aug 17, 2021

nathanaelbosch Aug 17, 2021

nathanaelbosch Aug 17, 2021

nathanaelbosch Aug 17, 2021

marvinpfoertner Aug 17, 2021

marvinpfoertner commented Aug 17, 2021

Remove globally disabled pylint messages #522

Remove globally disabled pylint messages #522

Conversation

marvinpfoertner commented Aug 16, 2021 • edited Loading

In a Nutshell

Detailed Description

Related Issues

codecov bot commented Aug 16, 2021 • edited Loading

Codecov Report

pnkraemer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinpfoertner Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinpfoertner commented Aug 17, 2021

Remove globally disabled `pylint` messages #522

Remove globally disabled `pylint` messages #522

marvinpfoertner commented Aug 16, 2021 •

edited

Loading

codecov bot commented Aug 16, 2021 •

edited

Loading

marvinpfoertner Aug 17, 2021 •

edited

Loading