Skip to content
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

Implement InsensitiveSet.__contains__ #1197

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Implement InsensitiveSet.__contains__ #1197

merged 2 commits into from
Feb 26, 2025

Conversation

quis
Copy link
Member

@quis quis commented Feb 19, 2025

I was surprised that "first name" in InsensitiveSet(['firstname']) evaluated to False.

This commit makes it work the way you’d expect.

We don’t use this in production code yet, but don’t think it can hurt.

@quis quis force-pushed the insensitive-set-contains branch from 85e3277 to b3ef620 Compare February 25, 2025 17:57
quis added 2 commits February 26, 2025 14:08
I was suprised that `"first name" in InsensitiveSet(['firstname'])`
evaluated to `False`.

This commit makes it work the way you’d expect.

We don’t use this in production code yet, but don’t think it can hurt.
@quis quis force-pushed the insensitive-set-contains branch from b3ef620 to b1da482 Compare February 26, 2025 14:09
@quis quis merged commit a6b6dd1 into main Feb 26, 2025
2 checks passed
@quis quis deleted the insensitive-set-contains branch February 26, 2025 14:13
@risicle
Copy link
Member

risicle commented Feb 26, 2025

This is the first time I've noticed InsensitiveSet - this certainly doesn't look like the only case where it won't behave correctly - e.g. no mutations after initial construction make any effort to normalize new values.

@quis
Copy link
Member Author

quis commented Feb 26, 2025

@risicle yeah it’s not used that widely. Think the only place at the moment is here: https://github.com/alphagov/notifications-admin/blob/2fc712d52176ddfc6ef3546fb42759614a1eca27/app/main/views/send.py#L806-L820

PRs to make it more correct welcome…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants