[pep8-naming] Avoid false positive for class Bar(type(foo)) (N804)#14683
[pep8-naming] Avoid false positive for class Bar(type(foo)) (N804)#14683AlexWaygood merged 8 commits intoastral-sh:mainfrom
pep8-naming] Avoid false positive for class Bar(type(foo)) (N804)#14683Conversation
|
| /// `IsMetaclass::No` if it's definitely *not* a metaclass, and | ||
| /// `IsMetaclass::Maybe` otherwise. | ||
| pub fn is_metaclass(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> IsMetaclass { | ||
| let mut maybe = false; |
There was a problem hiding this comment.
I don't really love having to capture maybe in the closure
hmm, yeah, this does feel a bit... icky 😄
I wonder if a better solution (though it's a bit more of a refactor) would be to split up any_base_class() into two functions: one that just handles iterating through all superclasses recursively (iter_base_classes() or iter_superclasses()), and one that checks if a condition holds true for any of the superclasses (that one could still be called any_base_class()). That way is_metaclass() could use the lower-level iter_base_classes() function directly rather than having to awkwardly work around the fact that the quite high-level any_base_class function doesn't really have quite the API we'd like it to have here.
WDYT?
There was a problem hiding this comment.
I think that's a great idea, I'll give it a try!
There was a problem hiding this comment.
Just some extra data point if it helps to avoid a lot of work: I don't mind that it it captures the variable
There was a problem hiding this comment.
This was a little harder than I thought. I've added an iter_base_classes function, but it's not truly an iterator now since I build up a Vec of base classes and then call into_iter. I'll keep thinking about this, but at least the API has the shape I was going for.
There was a problem hiding this comment.
I kind of prefer the old solution. It overall felt simpler and avoiding the extra allocation is nice too
There was a problem hiding this comment.
Yeah, writing a recursive iterator seems annoyingly hard :(
Sorry for leading you down the wrong track here @ntBre. My bad -- your first solution probably was best!
There was a problem hiding this comment.
No worries, it was interesting to try! I've reverted to the original.
One other thing I just noticed, should maybe also be true for the subscript case? I only handled the type(foo) example directly from the issue, but should type[foo] work as well? That's just a one-line code change, but I can update the tests too.
There was a problem hiding this comment.
should
maybealso betruefor the subscript case
I don't think so -- type[str], type[int], etc. always behaves the same as bare type in the context of a class's bases, and if a class inherits from type then it's always a metaclass
There was a problem hiding this comment.
Even subscripts that are "invalid" from a typing perspective behave the same way:
>>> class bar(type[int, str]): pass
...
>>> bar.__bases__
(<class 'type'>,)|
Also thanks for jumping on this! :D |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
Thanks all for your very speedy work getting this fixed! |
Summary
Fixes #14675, which reports N804 (
invalid-first-argument-name-for-class-method) for this code:I don't really love having to capture
maybein the closure, but this was the best way I could come up with to avoid propagating the newIsMetaclassreturn type throughany_base_class. Another option along these lines, which at least avoids changing theFns toFnMut, is to letmaybebe aRefCell<bool>and mutate it that way.Test Plan
cargo testwith the example above added toN804.py.