Pylint alerts corrections as part of an intervention experiment 401 #402
Conversation
The function _create_package uses os.mkdir and catch exception. This is too wide since mkdir catches specific exception. That might catch and hide new exception (e.g., in case that more code will be added to the try section). For the mkdir exceptions see: https://docs.python.org/3/library/os.html#os.mkdir
Removed unneeded parentheses in return statement
Removed unneeded parenthesis in
python_arg_scope = ("source.python meta.function-call.arguments.python string.quoted")
I suspected that the other might intended to create a tuple.
To create a single element tuple you need a comma at the end like
python_arg_scope = ("source.python meta.function-call.arguments.python string.quoted",)
So python_arg_scope was a string anyway
Also in line 262, in quite a similar code, there are no parenthesis.
Method run of class had 12 return statements (it is recommended by pylint not to have more than 6). I extracted methods that contains these statements. Since a return exit the function and a return in the new function will not exit from the caller, in some cases the result is checked in the caller, which returns if needed.
Function match_selector had 7 return statements while pylint recommends to have at most 6. I assigned the return values into the result variable and use a single statement returning in the end of the function.
The method on_query_completions of the class LegacySyntaxDefCompletions had 17 branches while pylint recommends to have at most 12. I extracted methods to structure more the code.
There was a problem hiding this comment.
Thanks for taking a shot at this.
Unfortunately, the two large refactoring commits 82a16c6 and 6b23492 introduced quite a few logic errors that were also not easy to spot during review and some of the function names choices are a bit unfortunate, which is likely because you're not familiar with the code and the problem domain (Sublime Text plugin). That is fine and all, but this review took me 45 minutes and that was honestly a bit more than I was anticipating.
I suggest to drop these two commits (or open a new PR without these two) and I will take a jab at refactoring the two functions myself. However, the syntax_dev_legacy.py file is, as the name indicates, in legacy territory and in maintenance mode, meaning I'm not really keen on spending time on it compared to addressing some of the already open issues.
Regardless, I hope you can put my detailed comments to use in your research.
plugins/file_conversion.py
Outdated
| # Save the file so that source and target file on the drive don't differ | ||
| self.view.run_command("save") | ||
| if self.view.is_dirty(): | ||
| result = sublime.error_message("The file could not be saved correctly. " |
There was a problem hiding this comment.
sublime.error_message returns None, so this breaks the logic in the calling function because it doesn't forward the function exit.
The various return some_function_call_that_returns_none are actually just shortcuts for some_function(); return because the return value of run is not inspected.
That's not clean code, obviously, but I wrote this over 10 years ago and didn't put code purity in as high of a regard as I do now. 😅
plugins/file_conversion.py
Outdated
| result = None | ||
| # Validate the shit again, but this time print to output panel | ||
| if source_format is not None and target_format == source_format: | ||
| result = output.print("\nTarget and source file format are identical. (%s)" |
There was a problem hiding this comment.
output.print returns None as well.
plugins/file_conversion.py
Outdated
| if target_format not in dumpers.get: | ||
| return output.print("\nDumper for '%s' not supported/implemented." | ||
| % target_format) | ||
| result = self._revalidate_run(self, |
There was a problem hiding this comment.
Remove self from the arguments.
plugins/file_conversion.py
Outdated
| return sublime.error_message("The file could not be saved correctly. " | ||
| "The build was aborted") | ||
|
|
||
| result = self._validate_run(self, source_format, target_format) |
There was a problem hiding this comment.
Remove self from the arguments.
By the way, you can also use the Python 3.8 walrus/assignment operator since this code targets Python 3.8 now.
plugins/file_conversion.py
Outdated
| output.write("Input type not specified, auto-detecting...") | ||
| for Loader in loaders.get.values(): | ||
| if Loader.file_is_valid(self.view): | ||
| source_format = Loader.ext |
There was a problem hiding this comment.
This source_format assignment is not returned.
| # None of our business | ||
| if not match_selector("- comment - (source.regexp - keyword.other.variable)"): | ||
| return None | ||
| result = None |
There was a problem hiding this comment.
Since result is already defined earlier, this could be a pass and can be interpreted as "don't change the default return value".
plugins/syntax_dev_legacy.py
Outdated
|
|
||
| return node | ||
|
|
||
| def _autocomplete(self, view, loc, window): |
There was a problem hiding this comment.
This function name is too generic. _build_variable_completions would be more fitting.
plugins/syntax_dev_legacy.py
Outdated
| result = [] | ||
| # Do not bother if not in yaml-tmlanguage scope and within or at the end of a comment | ||
| elif not view.match_selector(locations[0] | ||
| , "source.yaml-tmlanguage - comment"): |
There was a problem hiding this comment.
The comma should be on the previous line.
plugins/syntax_dev_legacy.py
Outdated
| # Do not bother if not in yaml-tmlanguage scope and within or at the end of a comment | ||
| elif not view.match_selector(locations[0] | ||
| , "source.yaml-tmlanguage - comment"): | ||
| result = [] |
There was a problem hiding this comment.
An empty list is falsy, so the return value will always be falsy and the check on the result in on_query_completions will never cause it to return.
plugins/syntax_dev_legacy.py
Outdated
| nodes = node.children | ||
| if not nodes: | ||
| break | ||
| node = self._browse_nodes(nodes, tokens, window) | ||
|
|
||
| if nodes and node: |
There was a problem hiding this comment.
With the current code, nodes will always be COMPILED_HEADS whereas previously it was modified during the loop.
The methods validate_data and write of DumperProto should be overridden by subclasses (as discussed with FichteFoll). Instead of leaving with just pass changed to raise NotImplementedError to verify that they are not mistakenly used.
The method parse of LoaderProto should be overridden by subclasses (as discussed with FichteFoll). Instead of leaving with just pass changed to raise NotImplementedError to verify that they are not mistakenly used.
This reverts commit 82a16c6.
This reverts commit 6b23492.
I did as you suggested, @FichteFoll. Note that I also changed "pass" to "raise NotImplementedError" in loaders and dumpers. Thank you for your help, @FichteFoll ! |
|
Thanks again. I have cherry-picked the non-problematic commits into another PR at #403 and merged it. |
Makes the interventions describe in intervention issue.
The experiment is described here.
Each intervention was done in a dedicated commit with a message explaining it.
The PR is still WIP.
I'd like to discuss some points in the issue.