classification
Title: ValueError for list.remove() not very helpful
Type: enhancement Stage: patch review
Components: Versions: Python 3.2
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, benjamin.peterson, brett.cannon, georg.brandl, rhettinger, tlesher
Priority: low Keywords: easy, patch

Created on 2008-12-03 23:38 by brett.cannon, last changed 2010-11-02 12:42 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
list-excs.diff georg.brandl, 2008-12-05 19:25 review
improve_list_remove_error.diff tlesher, 2010-07-21 19:08 review
Messages (8)
msg76852 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-12-03 23:38
If you try to remove something from a list, e.g. ``[].remove(1)``, the
message from ValueError is rather useless: "ValueError: list.remove(x):
x not in list". It should probably list the repr for the argument to
help in debugging.
msg77057 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-12-05 19:25
Suggested patch attached, handles remove() and index().
msg111076 - (view) Author: Tim Lesher (tlesher) * Date: 2010-07-21 15:27
It looks as if this has been addressed for list.index (aka issue #7252), in r76058.  The same fix could be applied for list.remove.
msg111106 - (view) Author: Tim Lesher (tlesher) * Date: 2010-07-21 19:08
This patch combines the fix from Georg Brandl's original patch with the fix made to listindex.  The r76058 fix fails the test in Georg's original test where the repr of the item to be removed itself raises; this patch handles that case for both list.remove and list.index.
msg111125 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-07-21 21:50
The patch is good, except for two things:
when PyObject_Repr() fails, the word 'item' is put instead. This is a good idea, but PyErr_Clear() should be called as soon as possible, before calling another API function.
Also, the error message can grow without bounds. It would be better to limit the size of the string.
msg111397 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-07-23 23:35
Amaury, I agree about having a limit.  It is a really bad idea to dump the full list repr for a potentially huge list or even for a small list of objects with huge individual reprs.   We should limit the output to just a few characters at most or risk spewing pages of garbage into people's tracebacks and log files.

That being said, limiting the output defeats the original purpose of the feature request.  ISTM that the proposal is only helpful in simple toy examples like [1,2,3].remove(4).  With real world data, the new message is probably going to cause more harm than good.  Accordingly, I recommend rejecting this feature request.
msg111615 - (view) Author: Tim Lesher (tlesher) * Date: 2010-07-26 13:41
Ugh.  That's a reasonable objection.

What's the best thing to do in this case, generally speaking?

list.index() does print the full repr on a value error; and a quick grep shows a number of other similar uses, although those don't seem to be as easy to trigger inadvertently.

Should this feature be removed from list.index as well?
msg120217 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2010-11-02 12:42
This has already been implemented.
History
Date User Action Args
2010-11-02 12:42:28benjamin.petersonsetstatus: open -> closed

nosy: + benjamin.peterson
messages: + msg120217

resolution: out of date
2010-07-26 13:41:18tleshersetmessages: + msg111615
2010-07-23 23:35:28rhettingersetversions: + Python 3.2, - Python 3.1, Python 2.7
nosy: + rhettinger

messages: + msg111397

type: behavior -> enhancement
2010-07-21 21:50:54amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg111125
2010-07-21 19:08:52tleshersetfiles: + improve_list_remove_error.diff

messages: + msg111106
2010-07-21 15:27:12tleshersetmessages: + msg111076
2009-03-10 11:21:59tleshersetnosy: + tlesher
2008-12-05 19:25:13georg.brandlsetfiles: + list-excs.diff
keywords: + patch
messages: + msg77057
nosy: + georg.brandl
stage: needs patch -> patch review
2008-12-03 23:38:36brett.cannoncreate