Support for pickling sentinel objects as singletons#617
Support for pickling sentinel objects as singletons#617HexDecimal wants to merge 2 commits intopython:mainfrom
Conversation
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 |
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.
|
|
Thanks @HexDecimal. I just tested your implementation in Pydantic to implement an |
This comment was marked as outdated.
This comment was marked as outdated.
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. |
This comment was marked as outdated.
This comment was marked as outdated.
93d7b65 to
de582a1
Compare
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:
|
|
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
e7ac070
Compare
e7ac070 to
678b73b
Compare
678b73b to
9d18f86
Compare
src/typing_extensions.py
Outdated
| def __init__( | ||
| self, | ||
| name: str, | ||
| module_name: typing.Optional[str] = None, |
There was a problem hiding this comment.
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.
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.
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
2238608
Compare
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