SessionBase.pop() no longer raises a KeyError

[Tobias Krönke]

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

[Tobias Krönke]

pull request is here: ​https://github.com/django/django/pull/6480

[Markus Holtermann]

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.

[Tobias Krönke]

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

[Nicolas Noé]

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'

[Tim Graham]

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)

[Tobias Krönke]

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.

[Nicolas Noé]

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?

[Tobias Krönke]

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