-
Notifications
You must be signed in to change notification settings - Fork 57
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
Code Linting with Pylint #150
Code Linting with Pylint #150
Conversation
Codecov Report
@@ Coverage Diff @@
## master #150 +/- ##
==========================================
+ Coverage 73.26% 73.32% +0.05%
==========================================
Files 57 57
Lines 3056 3040 -16
Branches 394 391 -3
==========================================
- Hits 2239 2229 -10
+ Misses 665 660 -5
+ Partials 152 151 -1
Continue to review full report at Codecov.
|
In general, I think all these issues raised should be fixed (obviously) but none in this PR specifically. For some there are already issues (e.g. for |
A few things that should be done as a part of this PR:
|
Closes #108 (and relates to #111).
Current state of the PR
I created configs, added a github action, and started fixing many errors. However I did not fix all issues, and often chose to add exceptions. I think that the best way would be to discuss these exceptions, fix those that should absolutely go into this PR, and then open specific issues about individual points.
Commentary for each of the exceptions:
invalid-name
: We will probably want to keep this, since we often want to use mathy notationno-else-return
,no-else-raise
: Not so clear to me what the benefit of fixing this isfixme
: Can be resolved through the separate issue Remove TODOs or change them to correct format #69too-many-arguments
: Appears accross modules. Good code style improvement, but not crucial. We could also raise the number of arguments from to 6 or 7 to resolve most warnigns, and to put the focus towards the cases with 12 (distribution.py
) and 10 (linearsolvers.py
) arguements.missing-module-docstring
,missing-function-docstring
: Could be considered as part of Automatically check whether docstrings are complete and correctly formatted #95, or we can also create a new issue. Although I have the impression that noone will do this at once.abstract-method
: Needs to be fixed inprobnum.linalg
andprobnum.prob
unused-argument
: Many of these are cases where**kwargs
gets passed around. I had the impression that this is often the "standard" in our code so I did not go ahead and fix it.too-many-branches
: Only affectslinearsolves.py
andmatrixbased.py
. I'd just open an issue for this specific point.arguments-differ
: Multiple cases inlinalg
, but also appearsx inprob
,quad
anddiffeq
redefined-builtin
: Inquad
andlinalg
; Fixing this will change some function signaturestoo-few-public-methods
: Mostlyquad
, but alsodiffeq.ODESolver
; Suggests considering removing these classestoo-many-locals
: Seems like a good tool to improve code, but is also non-trivial to fixno-self-use
: Suggests making functions instead of methodstoo-many-lines
: Suggests refactoringmatrixbased
broad-except
: Only appears innormal.py
. Should be easier to fix for someone familiar with that code.abstract-class-instantiated
: Only appears indiffeq
due to Nico's code structure; I think it would be best if he has a look.protected-access
: Something we probably want, but it's also not trivial or fast to fixduplicate-code
: Only appears infiltsmooth
too-complex
: McCabe complexity checker; Complains formatrixbased
too-many-instance-attributes
: Indicated thatmatrixbased.py
anddistribution
might need some refactoringtoo-many-statements
: Again inmatrixbased
attribute-defined-outside-init
: Mostly inmatrixbased
. One appearance inbayesquadrature.py
, but that code seems empty anyways.More exceptions for tests:
duplicate-code
,line-too-long
,missing-class-docstring
,unnecessary-pass
. I did not fix them becasue it did not feel too important for now, but especially theline-too-long
andunnecessary-pass
should be easy to fix quite mechanically. Alsounused-variable
, but in tests we often want to compute some value without actually using it afterwards, so that seems reasonable.Next steps
I think it would be good to merge a first version as soon as possible, and just open issues for the parts that we want to have fixed.
But it would be good to know if you think that we should still resolve one of the current exceptions in this PR.
If we decide to merge, I'll open individual issues for the remaining points.
Also, if you have any suggestion on how to better include pylint into our CI let me know!