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

Fix: #652: Resolve flake8 complaints #653

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Conversation

stennie
Copy link
Collaborator

@stennie stennie commented Jul 9, 2018

Description of changes

Tidy up per tox -e flake8

Testing

Ensure all tests passing on flake8, py27, and py36 envs

O/S testing:

O/S Version(s)
Linux
macOS 10.13.5
Windows

@stennie stennie requested a review from kallimachos July 9, 2018 10:49
Copy link
Collaborator

@kallimachos kallimachos left a comment

Choose a reason for hiding this comment

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

With the flake8 tests passing, do we want to add them to the basic tox envlist in line 3?

# F401 skipped (imported but unused) after verifying current usage is valid
# W503 skipped line break before binary operator
# C901 skipped: 'MLaunchTool.init' is too complex
ignore = E123,E125,N802,N806,F401,W503,C901
Copy link
Collaborator

Choose a reason for hiding this comment

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

An alternative to ignoring C901 is to set max-complexity = 55 below. Possibly a minor benefit in that it will flag a change that would raise the complexity even higher. Ideally, we would do some refactoring in the future to reduce the complexity of MLaunchTool.init, since it is twice as complex as the next most complex function (MLogFilterTool.run = 27). It is a low priority though, and it is fine to just ignore that test for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MLaunchTool.init should ideally be refactored in future, but given there weren't any other methods approaching this complexity I didn't think there was benefit in keeping the rule enabled.

builtins = _
exclude=.venv,.git,.tox,dist,*lib/python*,*egg,*figures/*,__init__.py
exclude=.venv,.git,.tox,dist,*lib/python*,*egg,*figures/*,__init__.py,build/*,setup.py,mtools/util/*,mtools/test/test_*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to ignore testing for mtools/util/*? Perhaps exclude for now, but raise an issue to fix violations at some point in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignoring for now, but will re-add in future via #655.

@stennie stennie merged commit f5dfeab into rueckstiess:develop Jul 10, 2018
@stennie stennie added this to the 1.5.2 milestone Jul 10, 2018
@stennie stennie deleted the flake8 branch October 29, 2022 09:54
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