classification
Title: {}.pop('a') raises non-standard KeyError exception
Type: behavior Stage: patch review
Components: Interpreter Core Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: belopolsky, djc, giampaolo.rodola, rhettinger
Priority: low Keywords: needs review, patch

Created on 2010-10-28 13:26 by djc, last changed 2010-11-01 18:29 by belopolsky. This issue is now closed.

Files
File name Uploaded Description Edit
issue10221.diff belopolsky, 2010-10-28 16:55
issue10221-with-tests.diff belopolsky, 2010-10-28 17:07
issue10221a-with-tests.diff belopolsky, 2010-10-28 17:21
Messages (10)
msg119778 - (view) Author: Dirkjan Ochtman (djc) * (Python committer) Date: 2010-10-28 13:26
djc@miles ~ $ python2.7
Python 2.7 (r27:82500, Oct  4 2010, 10:01:41)
[GCC 4.3.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> {}.pop('a')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'pop(): dictionary is empty'
>>> {'a': 'b'}.pop('c')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'c'

IMO the former exception should be in line with normal KeyErrors.
msg119796 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-10-28 15:44
There have been several requests for KeyError to grow "key" attribute that will always contain the key that caused the error. See issue 1182143, for example.  If this is done, I think it would be natural to unify the args as well for empty and non-empy case.  Without adding key access API, however, I am at most -0 on changing the error message.   Traditionally, the error messages are not considered part of language specification, so this is not a bug.
msg119802 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-10-28 16:37
I agree with the OP's request.

d.pop(k) is conceptually equivalent to:
   r = d[k]    # raises KeyError(k)
   del d[k]
   return r

The current message was probably borrowed from dict.popitem().  But that is much different since the dict.pop(k) method is key-specific (instead of size related).

This fix should be backported (as it doesn't change guaranteed behaviors but does improve the debugability).
msg119803 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-10-28 16:55
This is the case where fixing an issue is easier than arguing that it is not a bug. :-)  (Changing to "behavior" not because I agree that it is a bug, but for consistency with targeting 2.7)

A (1-line) patch attached.
msg119804 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-10-28 17:07
Attaching a patch with tests.
msg119805 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-10-28 17:21
I wonder if it would be worthwhile to unify missing key processing as in issue10221a-with-tests.diff.
msg119966 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-10-30 08:18
See r85967, r85968 and r85969.
msg120147 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-11-01 18:00
Raymond,

Did you mean to exclude unit test additions from your commit?  See issue10221-with-tests.diff.
msg120150 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-11-01 18:13
We don't usually test the content of error messages because they are not a guaranteed behavior.
msg120152 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-11-01 18:29
On Mon, Nov 1, 2010 at 2:13 PM, Raymond Hettinger
<report@bugs.python.org> wrote:
..
>
> We don't usually test the content of error messages because they are not a guaranteed behavior.
>
I recall that when I asked about this on #python-dev, someone pointed
to places where content of error messages is tested in python unit
tests.  I also asked whether such tests should be marked as cpython
details, and the answer was know.  Unfortunately I don't remember who
was telling me that, but it sounded reasonable enough that I submitted
the tests.

I think if testing error messages is a grey area, in this case it is
reasonable to have a test for at least two reasons:

1. It was reported as a bug, so users already expect this behavior.

2. There is no prose in the error message, just the key, so it is not
as arbitrary as other error messages.

Note that in my tests I deliberately tested only e.args[0] and not
str(e) or e.args[1:].
History
Date User Action Args
2010-11-01 18:29:20belopolskysetmessages: + msg120152
2010-11-01 18:13:21rhettingersetmessages: + msg120150
2010-11-01 18:00:11belopolskysetmessages: + msg120147
2010-10-30 08:18:02rhettingersetstatus: open -> closed
resolution: fixed
messages: + msg119966
2010-10-28 20:08:53rhettingersetresolution: accepted -> (no value)
stage: commit review -> patch review
2010-10-28 17:21:22belopolskysetfiles: + issue10221a-with-tests.diff

messages: + msg119805
2010-10-28 17:07:58belopolskysetfiles: + issue10221-with-tests.diff
messages: + msg119804

keywords: + needs review
resolution: accepted
stage: test needed -> commit review
2010-10-28 16:55:56belopolskysetfiles: + issue10221.diff
messages: + msg119803

keywords: + patch
type: enhancement -> behavior
stage: needs patch -> test needed
2010-10-28 16:38:09rhettingersetpriority: normal -> low
2010-10-28 16:37:50rhettingersetassignee: rhettinger
stage: needs patch
messages: + msg119802
versions: + Python 3.1, Python 2.7
2010-10-28 15:44:26belopolskysetversions: - Python 3.1, Python 2.7
nosy: + belopolsky

messages: + msg119796

type: behavior -> enhancement
2010-10-28 14:22:30giampaolo.rodolasetnosy: + giampaolo.rodola
2010-10-28 13:37:18pitrousetnosy: + rhettinger

type: behavior
versions: + Python 3.2, - Python 2.6
2010-10-28 13:27:04djcsetcomponents: + Interpreter Core
versions: + Python 2.6, Python 3.1, Python 2.7
2010-10-28 13:26:40djccreate