-
-
Notifications
You must be signed in to change notification settings - Fork 121
Refactor Sentinel to conform to PEP 661 #617
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
base: main
Are you sure you want to change the base?
Conversation
`repr` parameter removed, explicit repr tests removed `__repr__` modified to match PEP implementation (removed angle brackets) Added `module_name` parameter following PEP implementation and tweaking to use `_caller` helper function. `name` required support for qualified names to follow PEP implementation. Added `__reduce__` to track Sentinel by name and module_name. Added a Sentinel registry to preserve Sentinel identity across multiple calls to the class. Added tests for this. Added an import step to allow forward compatibility with other sentinel libraries. Import step is tested. This was not required by the PEP but it is required for typing_extensions to have a forward compatible type. Added copy and pickle tests. Updated documentation for Sentinel.
I think breaking changes are expected for draft PEPs, so imo no deprecation process should be used. Users should be aware that changes are expected. However, I think we can:
|
cc @taleinat |
I'm not willing to remove the |
Then I will change If compatibility is important enough then I can also have the code assume Configuration like |
Adds tests for both the keyword and old positional repr parameters
Changes in Sentinel such as swapping it with a theoretical typing.Sentinel will affect private methods, so the callable used with `__reduce__` must be at the top-level to be stable.
Additional data stored as a dict to ensure backwards and forwards compatibility
0653d70
to
2c509de
Compare
Use pickles method of handing singleton objects. This is simpler and more predictable than a custom unpickle function. Anonymous sentinels can no longer be pickled and will raise PicklingError instead of TypeError.
2c509de
to
e29210a
Compare
I've replaced the I've attempted to add to the discussion of PEP 661. Anonymous sentinels bring a lot of issues which need to be addressed. Before this is merged I'd like to add a |
src/typing_extensions.py
Outdated
return None | ||
try: | ||
module = importlib.import_module(module_name) | ||
return operator.attrgetter(name)(module) |
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.
return operator.attrgetter(name)(module) | |
return getattr(module, name) |
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 suggestion breaks qualified names and current tests. operator.attrgetter
handles dots while getattr
does not.
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.
Relevant code removed during refactor.
Thanks @HexDecimal. I just tested your implementation in Pydantic to implement an |
Note on behavior regarding the sentinel registry and imported sentinels: class MISSING:
pass
assert Sentinel("MISSING") is MISSING # Sentinel named after an existing non-Sentinel object Which of these makes the most sense for the above code?
Edit: currently going with 2 to simplify the implementation. 2nd behavior involving registered sentinel parameters: MISSING = Sentinel("MISSING", repr="foo")
assert MISSING is Sentinel("MISSING") # Implicit 'repr' conflicts with previous 'repr'
assert MISSING is Sentinel("MISSING", repr="bar") # Explicit 'repr' conflicts with previous 'repr' Which of these makes the most sense for the above code?
Edit: currently going with 4 to not break anything. The truthiness of sentinels is still being discussed. One thing which is unclear is if conversions to bool are common enough to be an issue in the first place (because sentinels are normally checked using identity comparison and little else). It'd be interesting to experiment with the most strict option and disable conversions to bool entirely: def __bool__(self) -> Never:
raise TypeError(...) |
I feel strongly that 2 is the right behavior. Constructing a class shouldn't look around in the globals for other stuff that might be using the same name. |
That was necessary to handle unpickling until I finally implemented the correct reduce function, but at this point I can remove that code now and revert to the old behavior. I'd accept that the other options are unnecessarily handholdy. |
No longer needed due to a correctly implemented reduce method. Simplifies code and makes syntax more predictable.
Remove documentation implying that multiple definitions of Sentinel objects are a regular occurrence
My attempt at implementing PEP 661 since I was unhappy with #594. I'm hoping that this is a PR of decent quality and not just me desperately wanting pickle support.
Changes made to Sentinel:
, reverted,repr
parameter removed, explicit repr tests removed,repr
was rejected by the PEPrepr
is kept__repr__
modified to match PEP implementation (removed angle brackets)module_name
parameter following PEP implementation and tweaking that to use the local_caller
helper function.name
required support for qualified names to follow PEP and ensure forward compatibility, this is tested. Unless sentinels in class scopes are forbidden then qualified names are not optional.__reduce__
to track Sentinel by name and module_name.This is not from the PEP but it is required for typing_extensions to be forward compatible with any official sentinel type.
For a while I still supported the
repr
parameter and after following the PEP 661 discussion I ended up implementing the truthiness tweaks mentioned there, but then I ended up scrapping all of that so that I could follow the PEP more closely, but they would be easy to reintroduce later if desired.If you don't like the current
__reduce__
function relying on the class directly then I can modify it to rely on a private classmethod instead which will ensure that any later changes to__new__
or__reduce__
won't cause issues. That said it seems thename, module_name
parameters are one of the few things most have agreed on.The
_import_sentinel
staticmethod can be moved if desired.I'm unsure of how to mention version changes in the docs. Replacing
repr
withmodule_name
is technically a breaking change._sentinel_registry.setdefault
is potentially supposed to be protected with a lock but I'm unsure of if I can import thethreading
module without causing platform issues._sentinel_registry
could be replaced with a weak values dictionary but I'm unsure if that's necessary due to how permanent most sentinels are.