Skip to content

Hide Sentinel.UNSET values as None in lookup_default()#3224

Merged
Rowlando13 merged 2 commits intopallets:stablefrom
kdeldycke:hide-unset-in-lookup_default
Feb 27, 2026
Merged

Hide Sentinel.UNSET values as None in lookup_default()#3224
Rowlando13 merged 2 commits intopallets:stablefrom
kdeldycke:hide-unset-in-lookup_default

Conversation

@kdeldycke
Copy link
Collaborator

This PR address issue #3145, in which the UNSET sentinel is leaking into lookup_default() when users have overriding it.

This changes prevent lookup_default() to return UNSET as-is, and masquerade it as None. To account for this change in lookup_default() behavior, I had to update some internal checks around defaults.

Additionally, to prevent any side-effects, I added a couple of tests to fill the gaps in coverage of lookup_default(), especially around treatment of absence of value and value of absence.

Important

I did use LLMs (Claude Opus 4.6) in the process of creating this PR, but as a co-worker to criticize my changes and modifications, not as a vibe-coding agent. This PR was carefully written, edited and reviewed by me.

@kdeldycke kdeldycke added this to the 8.3.2 milestone Feb 20, 2026
@kdeldycke kdeldycke added bug f:parameters feature: input parameter types labels Feb 20, 2026
@kdeldycke kdeldycke force-pushed the hide-unset-in-lookup_default branch from fe2f919 to 2112621 Compare February 20, 2026 08:31
@kdeldycke kdeldycke linked an issue Feb 20, 2026 that may be closed by this pull request
@kdeldycke
Copy link
Collaborator Author

kdeldycke commented Feb 20, 2026

This PR replace AI-slop from: #3199, #3202, #3209, #3212 and #3215.

@kdeldycke kdeldycke force-pushed the hide-unset-in-lookup_default branch from 2112621 to aaf99a3 Compare February 20, 2026 08:52
@kdeldycke
Copy link
Collaborator Author

Also note that the unnatural None dance around the results of lookup_default were made to please mypy.

@davidism
Copy link
Member

davidism commented Feb 20, 2026

This is fine for the bug fix, but you're right that this is getting messy. We need to figure out a long term solution.

@kdeldycke
Copy link
Collaborator Author

@Rowlando13, should we merge? to main or stable?

@Rowlando13
Copy link
Collaborator

Main. @kdeldycke Getting this fix is good, but it would be better if we try to make larger fixes to simplify the issues Click has accumulated.

@Rowlando13
Copy link
Collaborator

9.0 should be next release.

@Rowlando13 Rowlando13 modified the milestones: 8.3.2, 9.0.0 Feb 20, 2026
@kdeldycke
Copy link
Collaborator Author

Looks like @davidism is in favor on Discord of getting 8.3.2 out first, which could include this fix, which as limited blast radius. 😁

kdeldycke added a commit to kdeldycke/click-extra that referenced this pull request Feb 21, 2026
@kdeldycke
Copy link
Collaborator Author

FYI, just tested this fix against my own Click Extra codebase and could not find any regression: kdeldycke/click-extra@4bbd387

@kdeldycke kdeldycke modified the milestones: 9.0.0, 8.3.2 Feb 21, 2026
@Rowlando13
Copy link
Collaborator

Sounds good.

@sankarcn

This comment was marked as off-topic.

@davidism

This comment was marked as off-topic.

@Rowlando13 Rowlando13 changed the base branch from main to stable February 27, 2026 21:13
@Rowlando13 Rowlando13 merged commit b6afbf0 into pallets:stable Feb 27, 2026
13 checks passed
@kdeldycke
Copy link
Collaborator Author

Thanks @Rowlando13 for the merge!

@kdeldycke kdeldycke deleted the hide-unset-in-lookup_default branch February 28, 2026 06:28
kdeldycke added a commit to kdeldycke/click that referenced this pull request Mar 2, 2026
@kdeldycke
Copy link
Collaborator Author

UNSET is not a public API, so I'm not clear why you'd use that. Don't add the key to the default map if you don't want to change the default.

This is a reasonable assumption as in dict, you can materialize the absence of value by the absence of the key. But even if @sankarcn is a bot, it can still point to a gap in the tests around default_map. I will explore that in #3240

@sankarcn
Copy link

sankarcn commented Mar 2, 2026

@kdeldycke I am a human. Sorry, I could not reply to you and considered your reply as a guidance and I did not have anything to add. This was one of the edge cases, I came across when testing the PR, for a different purpose.

@kdeldycke
Copy link
Collaborator Author

@kdeldycke I am a human. Sorry, I could not reply to you and considered your reply as a guidance and I did not have anything to add. This was one of the edge cases, I came across when testing the PR, for a different purpose.

Ahah sorry, paranoia is high these days among open-source maintainers. Maybe you can tell us more about this use case. If you stumble upon it, there is a high chance others will. So I'd like to make a case so we can reduce the blast-radius of UNSET. Let's discuss this at: #3240

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug f:parameters feature: input parameter types

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lookup_default returns Sentinel.UNSET instead of None

4 participants