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

make Pylint checks much stricter #261

Merged
merged 18 commits into from
Jan 8, 2024
Merged

make Pylint checks much stricter #261

merged 18 commits into from
Jan 8, 2024

Conversation

Christian-B
Copy link
Member

@Christian-B Christian-B commented Jan 3, 2024

These PRs make the pylint tests much sticker and fix the errors found.
Increase the amount of pylint messages that cause an error by adding an exitcheck yto the yaml file

For all PRs related:
These we already check for but didn't error on
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/wrong-spelling-in-docstring.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/invalid-characters-in-docstring.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/information/c-extension-no-member.html

Most others checks listed below are disabled in the local .pylintrc

So far for only spinn_ultis the .pylintrc has been updated toremove all but a few exception listed below
With plans to add other repositories later:

These are ones found that I chose to fix.
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/wrong-spelling-in-comment.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/missing-function-docstring.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/wrong-import-order.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/consider-using-f-string.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/superfluous-parens.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/bad-mcs-classmethod-argument.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/missing-class-docstring.html
https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/single-string-used-for-slots.html

fix here but agreed to disable in other repsotories.
https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/unnecessary-dunder-call.html

These I added the check for these but often locally disabled as good to comment that is is an exception!
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/invalid-name.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/import-outside-toplevel.html

These pylint checks are globally disabled:

https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/consider-using-dict-items.html
Would require going from
for x in foo:
to
for x in foo.items()

https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/missing-module-docstring.html
Requires a comment at import level.
We do the comments at class level.

https://pylint.readthedocs.io/en/stable/user_guide/messages/error/unsubscriptable-object.html
-already disabled due to pylint-dev/pylint#1498
-better handled by mypy

I disabled pylint in spinn_utilities/citation as they are likely to be removed.

tested by
http://apollo.cs.man.ac.uk:8080/blue/organizations/jenkins/Integration%20Tests/detail/pylint_strict/1/pipeline

@coveralls
Copy link

coveralls commented Jan 4, 2024

Coverage Status

coverage: 88.665% (+0.008%) from 88.657%
when pulling 6ef7572 on pylint_strict
into 1003c12 on master.

@Christian-B
Copy link
Member Author

Christian-B commented Jan 5, 2024

@Christian-B
Copy link
Member Author

This PR was accidental merged without review
SpiNNakerManchester/TestBase#52

@Christian-B Christian-B merged commit 2fd5013 into master Jan 8, 2024
9 checks passed
@Christian-B Christian-B deleted the pylint_strict branch January 8, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants