8000 bpo-41817: use new StrEnum to ensure all members are strings by ethanfurman · Pull Request #22348 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-41817: use new StrEnum to ensure all members are strings #22348

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 22, 2020
Merged

bpo-41817: use new StrEnum to ensure all members are strings #22348

merged 2 commits into from
Sep 22, 2020

Conversation

ethanfurman
Copy link
Member
@ethanfurman ethanfurman commented Sep 22, 2020 •
edited by bedevere-bot
Loading

@ethanfurman
Copy link
Member Author

Terry, if the default for stdlib __str__ for stdlib enums changes to module.enum_member_name are you okay with changing the custome __str__ you are using for the EventType enum at the moment?

@serhiy-storchaka
Copy link
Member

I think it would be better to restore the default __str__: __str__ = str.__str__ (it looks not safe to change __str__ of str subclass) and change self.type in Event.__repr__() to getattr(self.type, 'name', self.type).

@terryjreedy
Copy link
Member
terryjreedy commented Sep 22, 2020 •
edited
Loading

Serhiy is indeed the one to answer the question as he is the tkinter maintainer. As a user, my concern is with breaking existing code and the possible uses of str(). EventTypes are used for the event.type attributes of events passed to event handlers. If a handler can be triggered by more that one type of event and the handler cares which, one condition would be event.type == EventType.Sometype, for which the string representations matter not. (I would probably define `Ev = tk.EventType if doing much of this.)

Conditions like str(event.type) == 'KeyPress' would break, and I don't consider their existence to be out of the question, as we use strings to bind the event. Before this issue, I might have done this. (I had to experiment to be sure of how to use the enum 'properly'.) The NMT tkinter reference does not mention EventType. And, of course, the mapping and code that uses the names predates enums. For output to a user, the module prefix might b 8000 e noisy.

@ethanfurman
Copy link
Member Author
ethanfurman commented Sep 22, 2020 •
edited
Loading

Terry, fair enough.

Serhiy, there are no issues with a custom __str__ on an Enum class, even when it is subclassing str. The question is whether we want str(EventType.KeyPress) to be "KeyPress" or "2".

As Serhiy explained on Python Dev: The issue with having a custom __str__ on a string subclass is that

str(subclassed_thing) != subclassed_thing

and it should be.

@serhiy-storchaka
Copy link
Member

I am also the one who introduced EventType. And I now think that maybe making __str__ returning a name instead of value was mistake. The main reason of defining __str__ was to use it in Event.__repr__().

But this is a different issue (I am working on it, the hard part is writing tests). This issue is about trailing commas which convert values into tuples. Thank you Ethan for catching this! Let to fix it.

@serhiy-storchaka
Copy link
Member

Would be nice to add a NEWS entry. This was a serious bug.

@ethanfurman
Copy link
Member Author

Adding news entry.

@terryjreedy
Copy link
Member

I cannot think of any use for the arbitrary numbers, whether string or int, added by tkinter to the tk strings.

@ethanfurman
Copy link
Member Author

Serhiy, thank you for the explanation on Python-Dev about str() vs __str__. I'm fixing StrEnum now to use str.__str__.

@ethanfurman ethanfurman merged commit ea0711a into python:master Sep 22, 2020
@miss-islington
Copy link
Contributor

Thanks @ethanfurman for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @ethanfurman for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 22, 2020
@ethanfurman @miss-islington
bpo-41817: use new StrEnum to ensure all members are strings (pythonG…
34dc7b0 
…H-22348)

* use new StrEnum to ensure all members are strings
(cherry picked from commit ea0711a)

Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
@bedevere-bot
Copy link

GH-22366 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 22, 2020
@ethanfurman @miss-islington
bpo-41817: use new StrEnum to ensure all members are strings (pythonG…
d5f9f06 
…H-22348)

* use new StrEnum to ensure all members are strings
(cherry picked from commit ea0711a)

Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Sep 22, 2020
@bedevere-bot
Copy link

GH-22367 is a backport of this pull request to the 3.9 branch.

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@ethanfurman
bpo-41817: use new StrEnum to ensure all members are strings (pythonG…
3dbc1bb 
…H-22348)

* use new StrEnum to ensure all members are strings
@ethanfurman ethanfurman deleted the enum-tkinter_eventtype branch April 15, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@terryjreedy terryjreedy Awaiting requested review from terryjreedy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ethanfurman @serhiy-storchaka @terryjreedy @miss-islington @bedevere-bot @the-knights-who-say-ni
0