[POC] all: fix deepsource checks #2016
Conversation
Refactor unnecessary `else` / `elif` when `if` block has a `raise` statement
Refactor unnecessary `else` / `elif` when `if` block has a `return` statement
Refactor unnecessary `else` / `elif` when `if` block has a `break` statement
Replace ternary syntax with if expression
Remove methods with unnecessary super delegation.
Refactor the comparison involving `not`
Remove unnecessary lambda expression
Remove unused imports
Use literal syntax instead of function calls to create data structure
|
Almost a year now, and I don't think we gave it a proper review. Probably because, as you may have guest, I'm not a huge fan of adding yet another tools with yet another opinions on code style. I think it's fair to say we can close this. What do you say @dgw ? |
There was a problem hiding this comment.
I'm not a huge fan of adding yet another tools with yet another opinions on code style.
What do you say @dgw ?
I say what's below in the line notes. tl;dr: This tool does have some useful rules, but I can only see documentation on ignoring issues in specific lines. One reason I think flake8 has been our preference for code style is that it lets you globally decide that you don't care about a certain style rule, and in many cases there are multiple rules to choose from (e.g. putting binary operators at the beginning or end of a line).
If there's really no way to customize the style rules applied by DeepSource, then the way forward is to look for flake8 plugins that enforce the DeepSource rules I/we do like and add those (to the wishlist in #1765, probably, so they can be implemented at an opportune moment).
| else: | ||
| channel = self._trigger.sender | ||
| channel = self._trigger.sender |
| self.privileges = dict() | ||
| self.privileges = {} |
There was a problem hiding this comment.
I don't mind enforcing this kind of style.
| if '*' in disabled_plugins: | ||
| return | ||
| elif rule.get_plugin_name() in disabled_plugins: | ||
| if rule.get_plugin_name() in disabled_plugins: | ||
| return |
There was a problem hiding this comment.
This, on the other hand, is silly. If the tool isn't smart enough to recommend merging the two conditions into one (as below), it shouldn't flag anything. Just using elif shouldn't be a style error, and there are numerous places in this PR where it's changed to plain if for no reason.
if (
'*' in disabled_plugins
or rule.get_plugin_name() in disabled_plugins
):
return| else: | ||
| return os.path.dirname(self.filename) | ||
| return os.path.dirname(self.filename) |
There was a problem hiding this comment.
But this kind of thing is also good.
| not current_name == new_name): | ||
| current_name != new_name): |
dgw
added
Declined
Description
Cleans up a bunch of possible style issues per https://deepsource.io/gh/RhinosF1/sopel/issues/?category=recommended
This is a proof of concept but if you like the feedback then I can go further or it could be configured on your end too.
Checklist
make qa(runsmake qualityandmake test)