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
Enum._convert shadows members named _convert #78463
Comments
If an enum has a member named _convert it gets shadowed by the _convert method as shown below. >>> import enum
>>>
>>> class Test(enum.Enum):
... _convert = enum.auto()
...
>>> Test._convert
<bound method Enum._convert of <enum 'Test'>> I've came up with a couple of solutions:
I think the first solution would be the worst as it would break existing enums that have _convert as a member (although unusable). The problem with the second solution would be breaking external code that uses it although I think that's a moot point as it's meant for internal use. Another shortcoming would be having to update all the stdlib code that uses it. The third solution might be a bit confusing on its own if an existing enum with a _convert member suddenly removed it leaving you with a bound method instead of raising an AttributeError. |
For versions 3.6 and 3.7 the solution is to modify the shadow check at line 236 so only DynamicClassAttributes are /not/ shadowed (meaning the _convert method would be shadowed by an _convert member). For 3.8 we can move _convert to the metaclass: I wasn't originally supportive of this idea, but I can see it being useful for other Enum mix-ins such as str. However, I will want to include a check that such a mix-in is there -- probably by checking that the Enum type is a subclass of the first member's type... something like: if not issubclass(cls, members[0]):
raise TypeError(...) While we're making that change we can also check that members is not empty and issue a friendlier error message. We can also rename _convert to _convert_ in 3.8, but we'll need to have _convert also on the metaclass and have it trigger a warning that _convert_ is now the right way, and _convert will go away in 3.9. |
Doing that might not be backwards-compatible. An enum with a subclassed behavior and member with the same name would shadow the behavior in favor of the member with the change. I propose adding an extra if statement under it that sets the member if it's named _convert and the _convert method on the class belongs to Enum (so it's not a behavior).
My thought for moving _convert to the metaclass was that it didn't make much sense for it to be accessible through a member. Could you elaborate on how mix-ins come into play?
I don't quite follow, what would members be in this case?
Sounds good. |
On 09/01/2018 01:21 AM, orlnub123 wrote:
Fixing bugs is often not backwards-compatible. ;) I think the number
Blocking access through the member is a low priority. For example, Also, moving _convert to the metaclass means it is now available on Enum, The reason _convert exists at all is to be able to convert sets of global
_convert is given a list of potential objects and a filter function to |
I think we had a misunderstanding, my proposal should be fully backwards-compatible. Maybe code can talk clearer than words? I'll attempt to turn it into a patch for 3.7 that can be backported to 3.6.
That's honestly very interesting, thanks for the insight.
In my testings _convert has always been available to them as they all subclass Enum where it's defined. Was it originally meant to be defined on IntEnum?
I understand and agree with the need for a check now however because of what I talked about above there should be a deprecation period similar to the one for renaming _convert. Would it be okay to add one?
My confusion came from the fact that it doesn't raise any errors currently if they're empty. Are you suggesting that it should raise one? |
orlnub123 wrote:
Wow, I really shouldn't check for things in the middle of the night. On the bright side, no further changes are needed to _convert besides the rename. orlnub123 wrote:
Please do. I'm curious to see what you come up with. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: