Skip to content

Correct the IDict pop declaration and the popitem docstring - #3824

Open
dchaudhari7177 wants to merge 1 commit into
Pylons:mainfrom
dchaudhari7177:fix/3237-idict-pop-signature
Open

dchaudhari7177 wants to merge 1 commit into
Pylons:mainfrom
dchaudhari7177:fix/3237-idict-pop-signature

Conversation

@dchaudhari7177

Copy link
Copy Markdown

Closes #3237.

pyramid.interfaces.IDict declares:

def pop(k, default=None):
    """... If k doesn't exist and default is not provided, raise a KeyError."""

The signature and the docstring contradict each other. With default=None
always bound there is no "default is not provided" case, so the KeyError
the docstring promises can never arise. dict.pop takes an optional second
positional argument with no default, so the declaration should be
pop(k, *args).

While in there I also fixed popitem, whose docstring described popping
"the item with key k" and raising KeyError when "k doesn't exist" —
but popitem() takes no arguments. It pops an arbitrary item and raises
KeyError when the dictionary is empty.

IDict is marked "Documentation-only interface", so this changes what is
published rather than any behaviour.

On the original report

I want to be straight about what I could and could not confirm. The issue
points at a PyPy verifyClass regression via a Bitbucket link that is now
dead, and the "temporary patch" PR #3183 was closed.

I could not reproduce a verification failure. On CPython dict.pop is a
C builtin with no introspectable signature, so zope.interface skips the
check entirely. I also tried a pure-Python pop(self, k, *args) implementer,
which is the shape PyPy would present — modern zope.interface accepts it
against both the old and the new declaration:

pop(k, default=None) vs pop(k,*args): OK
pop(k, *args)        vs pop(k,*args): OK

So whatever the 2018 failure was, current zope.interface no longer trips on
it, and I would not claim this PR fixes a PyPy crash.

What is still true, and is what the issue title actually says, is that the
declaration is wrong on its own terms. That is worth fixing for a
documentation interface regardless of whether any verifier currently objects.

Verification

pytest tests/ — 2528 passed, excluding test_docs.py and
test_integration.py, which need packages I do not have installed locally.
verifyClass(ISession, DummySession) still passes.

IDict declared pop(k, default=None), which contradicts its own docstring:
with a default always bound there is no "default is not provided" case, so
the KeyError it describes could never arise. dict.pop takes an optional
second positional argument with no default, so declare it pop(k, *args).

popitem's docstring described popping "the item with key k" and raising
KeyError when "k doesn't exist", but popitem takes no arguments. It pops an
arbitrary item and raises KeyError when the dictionary is empty.

IDict is a documentation-only interface, so this changes what is published
rather than any behaviour.

Closes Pylons#3237
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.

pyramid's IDict interface is incorrect for pop.

1 participant