Fix possibly-used-before-assignment issues in pylint codebase
#9644
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9644 +/- ##
==========================================
- Coverage 95.86% 95.84% -0.02%
==========================================
Files 174 174
Lines 18907 18898 -9
==========================================
- Hits 18125 18113 -12
- Misses 782 785 +3
|
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
added
Maintenance
|
Hey @aatle thank you for your contribution ! It would be better if each commit was passing the tests (i.e. first commit activates the check and disable warnings locally, then each following commit remove a local disable and fix the associated warning). That way we can look at each individual commit during review and partially revert or drop them later. You can open a merge request for each too if necessary. (Sometime it's nice to move what's consensual out of the way.) |
This comment has been minimized.
This comment has been minimized.
|
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit 165cfdd |
There was a problem hiding this comment.
Could you please open a first merge request with bea0aae and the new local disable first and the commit with no value error raised like 9e8d43f ? I think those change are mergeable as is This will make you trusted enough to run the CI without our approval and ease both of our lives and also make the following review smaller :)
| else: | ||
| raise ValueError(stat_type) |
There was a problem hiding this comment.
I think I'd rather have default value like
[
("convention", "NC"),
("refactor", "NC"),
("warning", "NC"),
("error", "NC"),
]
|
@aatle does this need a rebase? |
|
@DanielNoord I am not really sure (I'm not that experienced). I would think the best thing to do is to close this pull request and possibly the issue too. Merging this pull request wouldn't be that great, a new pull request to make these changes (if desired) would probably be better. If you agree, this PR can be closed and probably the original issue too, as the warnings have all been fixed (mostly with ignores). |
|
I'm fine with closing this, but I'm interested to see what is left after rebasing on |
|
The |
|
Do you think this PR can be closed now? My main thoughts is that the benefits are small, the code would still give an error either way, just a different type of error, since the changes are just raising explicit errors. The original issue can also probably be closed. |
|
Let's close the PR and issue :) |
Type of Changes
Description
This new check was disabled for pylint itself. Enabled, it gives 7 errors in 5 files in the codebase.
Enables the check for pylint and fixes the 7 errors.
Three of the errors were fixed by adding an
elseclause that explicitly raisedValueErrorfor bad values instead of letting the code possibly run into anUnboundLocalError.Two errors were caused by checking a bool flag (that never changes) in two separate conditionals: one where the variable set, the other where it was used.
The last two errors were caused by relying on a separate flag when setting a variable, for the conditional that uses the variable.
Closes #9637
Note: this is my first contribution to open source, so feedback and advice are welcome