Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 132
Support for pickling sentinel objects as singletons#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
HexDecimal commented Jun 4, 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.
python-cla-botbot commented Jun 4, 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.
Viicos commented Jun 5, 2025
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:
|
Viicos commented Jun 5, 2025
cc @taleinat |
JelleZijlstra commented Jun 5, 2025
I'm not willing to remove the |
HexDecimal commented Jun 5, 2025
Then I will change If compatibility is important enough then I can also have the code assume Configuration like |
2c509de to e29210aCompareHexDecimal commented Jul 2, 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.
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.
|
Uh oh!
There was an error while loading. Please reload this page.
Viicos commented Jul 2, 2025
Thanks @HexDecimal. I just tested your implementation in Pydantic to implement an |
This comment was marked as outdated.
This comment was marked as outdated.
JelleZijlstra commented Jul 4, 2025
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. |
HexDecimal commented Jul 4, 2025
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. |
This comment was marked as outdated.
This comment was marked as outdated.
93d7b65 to de582a1Comparecodecovbot commented Dec 2, 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.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@## main #617 +/- ## ======================================= Coverage 97.38% 97.38% ======================================= Files 3 3 Lines 7689 7701 +12 ======================================= + Hits 7488 7500 +12 Misses 201 201
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
HexDecimal commented Dec 2, 2025
I've narrowed the scope of this PR down to only what's needed to support pickling sentinels. Unfortunately this still means that Has anyone mentioned that |
de582a1 to e7ac070Comparee7ac070 to 678b73bCompare678b73b to 9d18f86Comparesrc/typing_extensions.py Outdated
| def__init__( | ||
| self, | ||
| name: str, | ||
| module_name: typing.Optional[str] =None, |
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.
Adding a new argument is problematic because it might (again) conflict with what PEP 661 proposes. I'd be OK with adding it but I'd make it keyword-only.
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.
I can make module_name keyword only, but what happens to repr? Do I revert repr back to a positional parameter?
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.
module_name is now keyword-only.
The `repr` parameters position was replaced by `module_name` to conform to PEP 661. Added copy and pickle tests. Updated documentation for Sentinel. `_marker` was defined before `caller` which causes minor issues, resolved by setting its module name manually.
9d18f86 to 2238608Compare
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:
module_nameparameter following PEP implementation and tweaking that to use the local_callerhelper function. Breaking change due toreprbecoming a keyword parameter.namerequires qualified names to support pickle singletons, this is tested and documented.__reduce__to track Sentinel by name and module_name.For a while I still supported the
reprparameter 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.I'm unsure of how to mention version changes in the docs. Replacing
reprwithmodule_nameis technically a breaking change.Closes#720