Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 132
Add support for sentinels (PEP 661)#594
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Viicos commented May 8, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
TeamSpen210 commented May 8, 2025
For pickling, there's some builtin behaviour which might be perfect for this. If |
JelleZijlstra commented May 9, 2025
I'm worried the pickling behavior the PEP wants may change over time. One option would be to make them not pickleable for now. That way, we can always add pickling as a feature later, but if we add pickling now and need to change the behavior later, that may cause issues for users. |
Viicos commented May 9, 2025
Yes anyway this will be better discussed in the PEP thread. I removed the |
JelleZijlstra commented May 9, 2025
Thanks! Can you make it raise an error? Otherwise I think it will pickle by value by default which is probably not what we'd want. |
Viicos commented May 9, 2025
Yes, makes sense, added a type error following prior art: https://github.com/search?q=repo%3Apython%2Fcpython+%22Cannot+pickle%22&type=code. |
JelleZijlstra left a comment
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.
Thanks! Could you also add this to the documentation?
Uh oh!
There was an error while loading. Please reload this page.
src/typing_extensions.py Outdated
| classSentinel: | ||
| """Create a unique sentinel object. | ||
| *name* should be the fully-qualified name of the variable to which the |
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.
Fully-qualified? That doesn't match the PEP which does MISSING = Sentinel("MISSING").
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.
This was copied verbatim from https://github.com/taleinat/python-stdlib-sentinels/, updated.
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.
Actually the repr construction by splitting dots don't make sense anymore, so I also updated the logic in the last logic.
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'd make sense to be fully qualified inside the module, so Sentinel("SomeClass.CONST"). Pickle expects that if we decide to use it. It's recommended in the PEP under "Additional Notes", but not required.
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'd make sense to be fully qualified inside the module, so
Sentinel("SomeClass.CONST"). Pickle expects that if we decide to use it. It's recommended in the PEP under "Additional Notes", but not required.
This will have to be decided in the PEP discussion.
JelleZijlstra commented May 13, 2025
Would you mind fixing the lint? I think we need to make |
479dae1 into python:mainUh oh!
There was an error while loading. Please reload this page.
HexDecimal commented Jun 2, 2025
This implementation of Sentinel is impractical in its current state. Sentinel's are supposed to be unique to the module it's defined in, allowing Sentinel's with the same name from separate modules to be equal is unexpected and broken behavior. importbar, baz# Both with 'MISSING = Sentinel("MISSING")'# Correct implementation would pass this assertassertbar.MISSING!=baz.MISSING# These modules mean "missing" in different ways!
importcopysentinel=Sentinel('sentinel') # Correct implementation would pass this assertassertsentineliscopy.copy(sentinel) # Any copy must have the same identity!An important feature of Sentinel's is that their identity is preserved even during serialization, but Pickle support seems to have been rejected. At the same time this implemented a >>>Sentinel("sentinel", repr="foo") ==Sentinel("sentinel", repr="bar") False# 'repr' makes these unique? |
JelleZijlstra commented Jun 3, 2025
The implementation doesn't do that. Since
In fact
We'll likely add Pickle support later but I do not believe it is essential. We're waiting for the PEP author to decide what it will look like.
Good point, I thought our implementation came from a proposed reference implementation. I'm not sure what the issues are that you're alluding to, but if the PEP doesn't end up supporting this argument we can deprecate it from typing-extensions. |
HexDecimal commented Jun 3, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Sorry. I've used dataclasses so often that I forgot the default behavior of Python classes. Here I am making several wrong assumptions about how equality works.
I've used sentinels in my own Python projects and pickling them is essential. I currently use sentinel-value but I'd wish to use something more standard. I don't really need to be in a hurry though.
Does this count? #591 (comment) |
Viicos commented Jun 3, 2025
@HexDecimal all this discussion should live in the PEP thread instead, where you can provide feedback on this initial implementation. Regarding pickling, I already started giving thoughts in the last comment of the thread. |
Fixes#591.
Regarding the pickling behavior, no matter if the sentinel is defined at the module level or in a nested scope, this seems tricky to handle. Ideally, I'd like to have the following working:
Considering unplicking data can be done across interpreter runs, this isn't really achievable, but still will be surprising.