-
Notifications
You must be signed in to change notification settings - Fork 42
add Flake8 plugins: flake8-isort & flake8_strict #86
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it locally:
flake8-isortdoes not require any changes, so I am fine with it.flake8-strictgenerates many false positives, makes the code worse readable (It tricked me to believe that[].append(3,)would append the tuple(3, ).), so I vote against using this plug-in.
| flags=0, | ||
| dont_inherit=False, | ||
| policy=RestrictingNodeTransformer, | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this one is less readable.
| type=v.__class__.__name__, | ||
| msg=v.msg, | ||
| statement=v.text.strip(), | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comma seems to be wrong. On the first look it seemed to me that a tuple from the string generated using .format() is appended.
I was wrong, but this hurts instead of it does help.
| flags=0, | ||
| dont_inherit=False, | ||
| policy=RestrictingNodeTransformer, | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
| converter = self.protect_unpack_sequence( | ||
| target, | ||
| ast.Name(tmp_name, ast.Load())) | ||
| ast.Name(tmp_name, ast.Load()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A trailing comma in lists, tuples or dicts is okay but having it in each function call is nonsense.
| reason="Print statement is gone in Python 3." | ||
| "Test Deprecation Warming in Python 2") | ||
| "Test Deprecation Warning in Python 2" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that this is more readable. Because of the nearly empty line above the function definition the decorator does not seem to belong to the function.
|
https://www.python.org/dev/peps/pep-0008/#when-to-use-trailing-commas could be read the way |
|
@loechel Are you still working on this PR or should we close it? |
Cleanup for readability.
#73 element as separate pull request.