Make used-before-assignment consider classes in method annotation and defaults
#5184
Conversation
… and defaults This closes pylint-dev#3771
DanielNoord
added
the
False Negative 🦋
Pull Request Test Coverage Report for Build 1373941910
💛 - Coveralls |
pylint/checkers/variables.py
Outdated
| self.add_message( | ||
| "used-before-assignment", node=node, args=node.name | ||
| ) | ||
| break |
There was a problem hiding this comment.
This for loop spanning 200 lines asks for a refactor. Maybe you have an idea on how we could simplify this ? I'm not saying we should refactor it now.
There was a problem hiding this comment.
It's awful. I don't even understand half of what it is doing and variable names such as maybee0601 really don't help.
One of the problems is that it is doing something with consuming variables in different scopes which I don't really understand. We can probably investigate this at some point.
I'm just going through the open issues for undefined-variable as it seemed like a good addition to 2.12 to improve that checker. For now I tend to add new single-purpose methods and elifs, that should help refactoring later.
There was a problem hiding this comment.
It's awful. I
Glad to see I'm not the only one 😄 Yes the consuming + continue + break makes the refactor quite hard. Also it's really hard to understand what's happening, good job fixing issues despite that 🎉
There was a problem hiding this comment.
The complete visit_name function has just gotten too complicated. Not sure we'll ever be able to fix it though. It's really easy to break things if something goes wrong during the refactor. Lot's of testing needed before then.
There was a problem hiding this comment.
It is also because the way consumption works is quite difficult to understand. It took me a while to understand how the actual looping over scopes works. If we were to improve that (or it explanation) I think we would already be in a much better place.
|
@Pierre-Sassoulas I just updated the method's name and docstring because I noticed they basically only referred to the checker for annotation and not for defaults. Should be good to go still! Edit: Github Actions failed on downloading python 3.10. Going to rerun! Edit2: Something wrong I guess.. Nothing on https://www.githubstatus.com yet. |
|
@Pierre-Sassoulas you approved this PR before Marc's review. I don't want to immediately tag him or ask for his re-review as not to spam him, but do you want him to take another look or can we merge this now? |
|
@DanielNoord I do plan on reviewing this shortly. Haven't had much time these past few days though. Sorry for the delay. |
|
@cdce8p No worries (and certainly no need to say sorry for any "delay")! I know you had requested some other reviews from yourself on some other PRs so I didn't want to unnecessarily bother you. I don't expect all maintainers/collaborators to be as active @Pierre-Sassoulas, we're all stil volunteers after all 😄 |
|
It's certainly interesting to keep up with you guys sometimes 😄 |
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
doc/whatsnew/<current release.rst>.Type of Changes
Description
This closes #3771
This was originally filed as
undefined-variablebut I think it should actually beused-before-assignment. I'm happy to change it toundefined-variableinstead.