-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #259--if a class has a non-trivial qualname, make sure to copy it when adding a metaclass #260
Conversation
six.py
Outdated
return metaclass(cls.__name__, cls.__bases__, orig_vars) | ||
new_cls = metaclass(cls.__name__, cls.__bases__, orig_vars) | ||
if hasattr(cls, '__qualname__') and cls.__qualname__ != cls.__name__: | ||
new_cls.__qualname__ = cls.__qualname__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not stick this in the orig_vars
dict rather than doing it after construction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I had it in my mind that that wouldn't work, since __qualname__
is a special slot. But I tried it, and you're right; that would work just a well.
test_six.py
Outdated
@six.add_metaclass(Meta) | ||
class B: pass | ||
|
||
assert A.B.__qualname__.endswith('A.B') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to make a stronger assertion (i.e., equality) that __qualname__
is correct. Perhaps, set __qualname__
on the original class to something known?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could actually go ahead and test it exactly. It's just slightly long in this case since it's a class defined inline in a function, so it ends up being test_add_metaclass_nested.<locals>.A.B
(which is still correct, at least).
Alternatively I could define the test class outside the function.
I simplified this a bit and got rid of the |
Ah, apparently |
…o copy it when adding a metaclass
Make the test more precise.
Thanks! |
Fixes #259.