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

Pylint and flake8 #214

Merged
merged 4 commits into from Feb 2, 2021
Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jan 29, 2021

Background

In #212 we switched to using pytest to handle our tests, since it can drive other tools with plugins, such as pytest-mypy which we also added in that PR.

Motivation

Pytest and mypy are nice, but we'd also like to use pylint and flake8.

Changes

setup.py:

  • pytest-flake8 and pytest-pylint are added to the [test] dependencies group

New parameters in the pytest.ini file:

  • -p no:cacheprovider turns off caching, because it just wastes CPU/IO in CI/CD (the cache folder is going to disappear at the end), and it causes false positives and false negatives locally if you edit which checks are disabled
  • --mypy --pylint --flake8 ensures that running pytest will always run mypy, pylint, and flake8. This way the individual dev doesn't have to know or remember which tools we use or how to call them; they'll all be pulled in automatically with a simpler command

Workflow step and Dockerfile:

  • Now runs pytest -v which will run everything thanks to the pytest.ini changes

.pylintrc:

  • Disabled the checks that are either too picky or would require large scale refactoring; we can re-enable some of these if we decide to refactor the affected code.

.flake8:

  • Disabled checks for whitespace before/after colons and long lines

Lots of minor changes to code to fix complaints from the still-enabled checks from both tools.

@HebaruSan HebaruSan added Enhancement New feature or request Tests Tests, we need more tests! labels Jan 29, 2021
Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall fantastic work! Couple of minor things, mostly due to some slight logic/formatting changes to appease the linters.

netkan/netkan/auto_freezer.py Show resolved Hide resolved
netkan/netkan/auto_freezer.py Show resolved Hide resolved
netkan/netkan/cli/common.py Show resolved Hide resolved
netkan/netkan/cli/services.py Show resolved Hide resolved
netkan/netkan/cli/utilities.py Show resolved Hide resolved
netkan/netkan/github_pr.py Outdated Show resolved Hide resolved
netkan/netkan/github_pr.py Outdated Show resolved Hide resolved
netkan/netkan/metadata.py Show resolved Hide resolved
@techman83
Copy link
Member

I added some fixes for VS Code.

  • VS Code runs the test from the top level. This is both annoying and handy.
  • Top level pytest.ini disables flake8/pylint/mypy. These are time consuming and the tests are already enabled within the editor, so they occur as you code.
  • Enabled code auto formating. This was disabled as there was quite a lot to change, which has now since been done.

@HebaruSan
Copy link
Member Author

Pushed fixes to the previous round of comments, including a new set of tests for AutoFreezer.

Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, as always, love your work! Let's merge this 😃

@techman83 techman83 merged commit 89b7d82 into KSP-CKAN:master Feb 2, 2021
@techman83 techman83 mentioned this pull request Feb 2, 2021
@HebaruSan HebaruSan deleted the feature/pylint-flake8 branch February 2, 2021 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Tests Tests, we need more tests!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants