#26520 closed Bug (fixed)
SessionBase.pop() no longer raises a KeyError
Reported by: | Tobias Krönke | Owned by: | nobody |
---|---|---|---|
Component: | contrib.sessions | Version: | 1.9 |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
The fix of #24621 led to always providing a default
to session's dict's pop()
method. This prevents a KeyError
from ever being raised, which should be expected by a pop-function.
Pull request is https://github.com/django/django/pull/6480
Change History (12)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Description: | modified (diff) |
---|
comment:3 by , 8 years ago
While I agree that .pop()
should raise a KeyError
on a missing key I don't think reverting that particular commit is the solution. We should rather fix the underlying bug than what covers it up.
comment:4 by , 8 years ago
Thanks for your feedback. Do you just propose to further change the docs or also even the code? I think the session.pop
wrapper should have a signature as close to the original dict.pop
as possible:
>>> {}.pop("key", default="fallback") Traceback (most recent call last): File "<bpython-input-6>", line 2, in <module> {}.pop("key", default="fallback") TypeError: pop() takes no keyword arguments
comment:5 by , 8 years ago
dict.pop
raise KeyError if the key is not found AND there's no default provided as second argument... Won't implementing the same in session.pop
solve both this issue and #24621 (given that the documentation is updated, too) ?
Example:
>>> {}.pop('machin') Traceback (most recent call last): File "<stdin>", line 1, in <module> KeyError: 'machin' >>> {}.pop('machin', 'truc') 'truc'
comment:6 by , 8 years ago
Patch needs improvement: | set |
---|---|
Severity: | Normal → Release blocker |
Summary: | Undo #24621 to make session.pop raise a KeyError again → SessionBase.pop() no longer raises a KeyError |
Triage Stage: | Unreviewed → Accepted |
How about using a pattern borrowed from cpython?
__marker = object() def pop(self, key, default=__marker): self.modified = self.modified or key in self._session args = () if default is self.__marker else (default,) return self._session.pop(key, *args)
comment:7 by , 8 years ago
Maybe, Django users out there are already using the named argument default
, so it would be bad to remove it again. Although the solution is more complex in code, it seems more explicit for documentation and code introspection. Either way, I am happy, that it will raise a KeyError
again.
comment:8 by , 8 years ago
I am currently trying timgraham's suggested implementation, and it seems to work well for all those cases:
(Pdb) session.pop('machin') *** KeyError: 'machin' (Pdb) session.pop('machin', 'truc') 'truc' (Pdb) session.pop('machin', default='truc') 'truc'
Should I make a pull request with this?
comment:10 by , 8 years ago
Description: | modified (diff) |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
pull request is here: https://github.com/django/django/pull/6480