Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
4 of 11 tasks
marvinpfoertner opened this issue Aug 16, 2021 · 4 comments
Open
4 of 11 tasks
Labels
feature request Requests for features to be implemented

Comments

@marvinpfoertner
Copy link
Collaborator

marvinpfoertner commented Aug 16, 2021

Is your feature request related to a problem? Please describe.
For legacy reasons, we disable subsets of pylint messages on a per-package basis (see tox.ini), since our code does not yet fully comply with the desired pylint configuration (see pyproject.toml).
Moreover, a bug in our CI configuration destroyed previous efforts to become pylint compliant, since non-compliant code could be merged without CI failure.
This bug is fixed by #522.

Describe the solution you'd like.
The code should be refactored such that we can re-enable the messages disabled on a per-module basis for good. Due to our CI configuration, this can be done independently by the individual package maintainers. We can use this issue to track our progress.
Compliant packages:

  • diffeq
  • filtsmooth
  • kernels
  • linalg
  • linops
  • problems
  • quad
  • randprocs
  • randprocs.markov
  • randvars
  • utils
@marvinpfoertner
Copy link
Collaborator Author

A complete list of pylint disables that require fixing:

  • no-member
  • import-error
  • abstract-method
  • abstract-class-instantiated
  • arguments-differ
  • arguments-renamed
  • function-redefined
  • redefined-builtin
  • redefined-outer-name
  • too-many-instance-attributes
  • too-many-arguments
  • too-many-locals
  • too-many-lines
  • too-many-statements
  • too-many-branches
  • too-complex
  • too-few-public-methods
  • protected-access
  • unnecessary-pass
  • unused-variable
  • unused-argument
  • unused-private-member
  • attribute-defined-outside-init
  • no-else-return
  • no-else-raise
  • no-self-use
  • consider-using-from-import
  • duplicate-code
  • line-too-long
  • missing-module-docstring
  • missing-class-docstring
  • missing-function-docstring

@pnkraemer
Copy link
Collaborator

How do people feel about raising the "too-many-arguments" threshold from 5 to a larger number?

I find 5 a bit tedious to comply with everywhere. For example, all the top-level interface functions in probnum seem to have more than 5. Many of the lower-level methods also depend on a number of parameters. Usually, most of them are optional anyway. It also feels like defeating the purpose to put "pylint: disable="too-many-arguments" everywhere.

@pnkraemer
Copy link
Collaborator

And, on a similar note, how about disabling "missing-function-docstring" for the tests? Most tests are one-liner-functions with (usually) a rather descriptive name. How much value does a (forced) docstring add there?

@marvinpfoertner
Copy link
Collaborator Author

How do people feel about raising the "too-many-arguments" threshold from 5 to a larger number?

I find 5 a bit tedious to comply with everywhere. For example, all the top-level interface functions in probnum seem to have more than 5. Many of the lower-level methods also depend on a number of parameters. Usually, most of them are optional anyway. It also feels like defeating the purpose to put "pylint: disable="too-many-arguments" everywhere.

And, on a similar note, how about disabling "missing-function-docstring" for the tests? Most tests are one-liner-functions with (usually) a rather descriptive name. How much value does a (forced) docstring add there?

@pnkraemer I quoted these comments in #470. Let's discuss there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requests for features to be implemented
Projects
Development

No branches or pull requests

7 participants