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

Use Pylint to catch some mistakes #54

Merged
merged 39 commits into from
Dec 8, 2018
Merged

Use Pylint to catch some mistakes #54

merged 39 commits into from
Dec 8, 2018

Conversation

ssssam
Copy link
Owner

@ssssam ssssam commented Dec 8, 2018

This adds a ninja pylint target that can be run by developers, and fixes lots of warnings that were showing up (and disables lots more).

Currently pylint is a hard dependency of calliope. In future it can be made optional, although a specific hard-disable flag would be best.

We could add a -Dtests=false option which would actually
remove this dependency, but currently if pytest is missing
things will just break.
This runs the Pylint code checker.
The 'gi.require_versions()' call seems to trigger this warning, but
we need to call that before importing from 'gi'.
I like having system imports after site-packages imports.
Some functions are self-documenting.
I can see by eye when the code is too complex, but I don't always have
time to fiddle around with it trying to satisfy the linter.
In general constants should have UPPER CASE names, but it looks ugly
if we call this one LOG.
Sometimes it looks clearer to explicitly write an 'else' clause, even if
it's not strictly necessary.
Occasionally a single-letter argument name is nice to have.
This is unhelpful when we have callbacks from 3rd party libraries
that supply parameters that we don't use.
@ssssam ssssam merged commit b2546d2 into master Dec 8, 2018
…nings

These are well intentioned, but I don't fancy fixing all that code right now.
@ssssam ssssam mentioned this pull request Dec 8, 2018
@ssssam ssssam deleted the sam/pylint branch December 8, 2018 00:36
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.

1 participant