-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Conversation
For pickling, there's some builtin behaviour which might be perfect for this. If |
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. |
Yes anyway this will be better discussed in the PEP thread. I removed the |
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. |
Yes, makes sense, added a type error following prior art: https://github.com/search?q=repo%3Apython%2Fcpython+%22Cannot+pickle%22&type=code. |
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?
src/typing_extensions.py
Outdated
class Sentinel: | ||
"""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.
Would you mind fixing the lint? I think we need to make |
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. import bar, baz # Both with 'MISSING = Sentinel("MISSING")'
# Correct implementation would pass this assert
assert bar.MISSING != baz.MISSING # These modules mean "missing" in different ways!
import copy
sentinel = Sentinel('sentinel')
# Correct implementation would pass this assert
assert sentinel is copy.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? |
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. |
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) |
@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.