Correct the IDict pop declaration and the popitem docstring - #3824
Open
dchaudhari7177 wants to merge 1 commit into
Open
dchaudhari7177 wants to merge 1 commit into
dchaudhari7177 wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3237.
pyramid.interfaces.IDictdeclares:The signature and the docstring contradict each other. With
default=Nonealways bound there is no "default is not provided" case, so the
KeyErrorthe docstring promises can never arise.
dict.poptakes an optional secondpositional 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 raisingKeyErrorwhen "kdoesn't exist" —but
popitem()takes no arguments. It pops an arbitrary item and raisesKeyErrorwhen the dictionary is empty.IDictis marked "Documentation-only interface", so this changes what ispublished 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
verifyClassregression via a Bitbucket link that is nowdead, and the "temporary patch" PR #3183 was closed.
I could not reproduce a verification failure. On CPython
dict.popis aC builtin with no introspectable signature, so
zope.interfaceskips thecheck entirely. I also tried a pure-Python
pop(self, k, *args)implementer,which is the shape PyPy would present — modern
zope.interfaceaccepts itagainst both the old and the new declaration:
So whatever the 2018 failure was, current
zope.interfaceno longer trips onit, 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, excludingtest_docs.pyandtest_integration.py, which need packages I do not have installed locally.verifyClass(ISession, DummySession)still passes.