Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#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 Tim Graham)

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 Tobias Krönke, 8 years ago

comment:2 by Tobias Krönke, 8 years ago

Description: modified (diff)

comment:3 by Markus Holtermann, 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 Tobias Krönke, 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
Version 0, edited 8 years ago by Tobias Krönke (next)

comment:5 by Nicolas Noé, 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'
Last edited 8 years ago by Tim Graham (previous) (diff)

comment:6 by Tim Graham, 8 years ago

Patch needs improvement: set
Severity: NormalRelease blocker
Summary: Undo #24621 to make session.pop raise a KeyError againSessionBase.pop() no longer raises a KeyError
Triage Stage: UnreviewedAccepted

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 Tobias Krönke, 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 Nicolas Noé, 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:9 by Tobias Krönke, 8 years ago

I already changed my pull request to reflect timgraham's idea.

comment:10 by Tim Graham, 8 years ago

Description: modified (diff)
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:11 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In b040ac06:

Fixed #26520 -- Fixed a regression where SessionBase.pop() didn't return a KeyError.

comment:12 by Tim Graham <timograham@…>, 8 years ago

In 845d43e3:

[1.9.x] Fixed #26520 -- Fixed a regression where SessionBase.pop() didn't return a KeyError.

Backport of b040ac06ebba2348cece7390b88f746d2c91d07b from master

Note: See TracTickets for help on using tickets.
Back to Top