astroid
astroid copied to clipboard
Fix `ClassDef.igetattr` on multiple attributes (`not-an-iterable` false-positive)
Steps
- [ ] For new features or bug fixes, add a ChangeLog entry describing what your PR does.
- [x] Write a good description on what the PR does.
Description
This is an attempt at fixing #1015; however I really don't know astroid
's code base so this is more like a shot in the dark, which needs a careful review!
In a few words:
class A:
def foo(self): ...
def foo(self):
yield
for _ in A().foo():
pass
Only the first implementation returning ...
is considered, resulting in pylint
complaining:
E1133: Non-iterable value A().foo() is used in an iterating context (not-an-iterable)
This is especially annoying when we define @overload
variants for typing purposes.
Type of Changes
Type | |
---|---|
✓ | :bug: Bug fix |
Related Issue
Closes #1015
Ping @nelfin @belm0 who are probably interested in this.
thank you for working on it
I think it will need unit test coverage. It's good to write the test and verify that it fails before applying the fix.
Thank you for the feedback, I have added a test and confirmed that it failed before the fix.
Pull Request Test Coverage Report for Build 3404098486
- 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.0007%) to 92.274%
Totals | |
---|---|
Change from base Build 3393053917: | 0.0007% |
Covered Lines: | 9865 |
Relevant Lines: | 10691 |
💛 - Coveralls
Thanks @Rogdham, did you give any thought to the comment on the previous attempt?
Dammit I completely missed #1173 for some reason, this would have saved me some headache :grimacing:
Since their fix looks the same to me, should I just close this PR? They seems to know what they are talking about way more than I do!
I'd see if the author responds soon, but since it's been over a year, feel free to pick up where they left off later this month.
I never did figure out how to address those comments within the scope of that change and then I spent some time away from contributing to open source for a while. Feel free to cherry-pick those tests if you think they will help. If I remember correctly and understand this change, the comment that Klass.foo
should only be inferred as Generator
would necessitate a change here as len(attrs)
should be 1 in that case, not 2.
Superseded in #1173