Skip to content

fix: IntervalDict.pop(key, None) raises KeyError instead of returning None#113

Closed
gaoflow wants to merge 1 commit into
AlexandreDecan:masterfrom
gaoflow:fix/intervaldict-pop-none-default
Closed

fix: IntervalDict.pop(key, None) raises KeyError instead of returning None#113
gaoflow wants to merge 1 commit into
AlexandreDecan:masterfrom
gaoflow:fix/intervaldict-pop-none-default

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 25, 2026

Copy link
Copy Markdown

What

IntervalDict.pop(key, default) uses default is None to detect whether a
default argument was supplied, so passing None explicitly is indistinguishable
from calling pop(key) with no default:

d = P.IntervalDict([(P.closed(0, 5), 'A')])

d.pop(10)        # KeyError — correct
d.pop(10, None)  # KeyError — wrong: should return None
d.pop(10, 'x')  # 'x'     — correct

Python's built-in dict.pop always returns the default when one is given,
regardless of its value.

Fix

Replace the None sentinel with a private _MISSING = object() sentinel so
that any explicitly supplied default — including None — is honoured.

Test

Added an assertion to test_pop_missing_value that pop(key, None) returns
None for a missing key rather than raising KeyError.


This pull request was prepared with the assistance of AI, under my direction and review.

… None

`pop(key, default)` used `default is None` to detect whether a default
was provided, so `pop(key, None)` was treated the same as `pop(key)` and
raised KeyError for missing keys. Replace the `None` sentinel with a
private `_MISSING` object so any explicitly supplied default — including
`None` — is respected.

Fixes behaviour to match Python's built-in `dict.pop(key, default)`:
- `pop(key)` with no default → KeyError if missing (unchanged)
- `pop(key, None)` → returns `None` for missing key (was: KeyError)
- `pop(key, val)` → returns `val` for missing key (unchanged)
@AlexandreDecan

Copy link
Copy Markdown
Owner

Thanks for proposing this "fix" (it wasn't an issue since the behaviour aligns with the documentation). Was there a human involved in the loop? Do you use this library? Your recent activity suggests (creating hundreds of PRs in dozens of unrelated projects) this was fully automatically generated by an AI.

@gaoflow

gaoflow commented Jun 25, 2026

Copy link
Copy Markdown
Author

Thanks for calling this out. You're right that I misread the documented contract here, so this PR should not be treated as a bug fix.

There was human review in the loop, but the patch was AI-assisted and I do not use this library in production. I should have been more conservative before opening the PR. Closing this to avoid taking more maintainer time.

@gaoflow gaoflow closed this Jun 25, 2026
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.

2 participants