Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
May 13, 2025
Merged

Add support for sentinels (PEP 661) #594

merged 8 commits into from
May 13, 2025

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented May 8, 2025 •
edited
Loading

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:

sentinel = Sentinel('sentinel')

class A:
    def __init__(self):
        self.s = sentinel

a = A()
a.s is sentinel  # True

import pickle

a_r = pickle.loads(pickle.dumps(a))

a_r.s is sentinel  # False

Considering unplicking data can be done across interpreter runs, this isn't really achievable, but still will be surprising.

@TeamSpen210
Copy link

For pickling, there's some builtin behaviour which might be perfect for this. If __reduce__ returns the sentinel name, and __module__ is defined (via sys._getframemodulename()), pickle will import the module then lookup attributes to find the value. It'll even check and error on pickling if the sentinel isn't present there. This'll make your example work, by looking up __main__.sentinel. If that global wasn't present, self.s = Sentinel("a.s") would also work.

@JelleZijlstra
Copy link
Member

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
Copy link
Contributor Author

Viicos commented May 9, 2025

Yes anyway this will be better discussed in the PEP thread. I removed the __reduce__() implementation, let me know if you actually want to raise an error.

@Viicos Viicos marked this pull request as ready for review May 9, 2025 08:45
@JelleZijlstra
Copy link
Member

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
Copy link
Contributor Author

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
JelleZijlstra reviewed May 9, 2025
View reviewed changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a 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 Show resolved Hide resolved
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
Copy link
Member

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").

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Contributor Author

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
Copy link
Member

Would you mind fixing the lint? I think we need to make Union unconditionally defined.

@JelleZijlstra JelleZijlstra merged commit 479dae1 into python:main May 13, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TeamSpen210 TeamSpen210 TeamSpen210 left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for PEP 661 sentinels
3 participants
@Viicos @TeamSpen210 @JelleZijlstra
Morty Proxy This is a proxified and sanitized view of the page, visit original site.