-
Notifications
You must be signed in to change notification settings - Fork 96
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
Bump pylint and flake8 #232
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -279,8 +279,8 @@ def _get_op_handler(operation): | |
if isinstance(op, types.FunctionType): | ||
return op | ||
return six.get_method_function(op) | ||
except (ValueError, AttributeError): | ||
raise ValueError("The operation '{}' is invalid.".format(operation)) | ||
except (ValueError, AttributeError) as ex: | ||
raise ValueError("The operation '{}' is invalid.".format(operation)) from ex | ||
Comment on lines
+281
to
+282
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use Python's built-in Exception Chaining as recommended by W0707:
|
||
|
||
def deprecate(self, **kwargs): | ||
kwargs['object_type'] = 'command group' | ||
|
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.
It's a big change for our code. Can you provide more information about this.
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.
If we run
pylint==2.6.0
on the current code, it errors out:The help message is pretty straight-forward:
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.
As I know, python2 does not support this, and pycharm are using the old format in code autocompletion.
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.
We have dropped Python 2 support long time ago (#174). PyCharm is only an IDE and is not the standard. We had better follow PEP 3135 instead.
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.
We need to make the same changes for
azure-cli
andazure-cli-extensions
after using the new knack version with this bump?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.
Definitely not. Knack itself doesn't require pylint and flake8. This PR only makes sure Knack complies with the latest pylint and flake8.
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 see. pylint is still pinned to 2.3.0 in azdev.