python / cpython

The Python programming language
https://www.python.org
Other
63.46k stars 30.39k forks source link

Add a 'key' attribute to KeyError #62363

Open brettcannon opened 11 years ago

brettcannon commented 11 years ago
BPO 18163
Nosy @warsaw, @brettcannon, @rhettinger, @aroberge, @ezio-melotti, @bitdancer, @sblondon, @zingero, @p4m-dev

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-feature'] title = "Add a 'key' attribute to KeyError" updated_at = user = 'https://github.com/brettcannon' ``` bugs.python.org fields: ```python activity = actor = 'aroberge' assignee = 'none' closed = False closed_date = None closer = None components = [] creation = creator = 'brett.cannon' dependencies = [] files = [] hgrepos = [] issue_num = 18163 keywords = [] message_count = 12.0 messages = ['190778', '190782', '190786', '190791', '190999', '191001', '191038', '191083', '378075', '378253', '383445', '386890'] nosy_count = 10.0 nosy_names = ['barry', 'brett.cannon', 'rhettinger', 'aroberge', 'ezio.melotti', 'r.david.murray', 'cvrebert', 'sblondon', 'zingero', 'platonoff-dev'] pr_nums = [] priority = 'normal' resolution = None stage = 'test needed' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue18163' versions = [] ```

brettcannon commented 11 years ago

KeyError could grow a 'key' attribute for the key that triggered the exception. Since keys are expected to be immutable (in order to be hashable) there is no GC issue to worry about.

bitdancer commented 11 years ago

I don't see how the fact that keys are immutable implies there are no GC issues. A tuple can be involved in a cycle, for example.

brettcannon commented 11 years ago

So are you arguing it should be a weakref, or just saying you view the statement as false?

bitdancer commented 11 years ago

I'm arguing that the statement is false. I think that whether or not it should be a weakref in this and the other cases depends on whether you think an exception object should keep an object alive or not. It is fairly unlikely that a key would get into a cycle with an error message, though certainly not impossible.

The "keep alive" question probably boils down to whether or not we want it to be the case that clearing the traceback attribute releases all the "extra" pointers an exception holds on to, or if it is acceptable that an arbitrary number of additional attributes might do so.

I'm inclined to think that using weakrefs would make using the attributes more complicated for relatively little gain.

rhettinger commented 11 years ago

Since the key is already accessible via the "args" attribute, what is the point of a new attribute?

>>> d = {}
>>> try:
    d['roger']
except KeyError as e:
    print(e.args)

('roger',)

bitdancer commented 11 years ago

Making it unambiguous what piece of data is being retrieved, and allowing new code to have a more complex message than just 'Keyerror: xxxx' and still be able to get at only the missing key.

brettcannon commented 11 years ago

What David said. =)

The fact that the key is the first value from args is almost happenstance as the exception message is customized in str() and not in what is passed to the exception. But what if I had defined getattr or getattribute and had some prefix requirement? It would be more helpful to say KeyError("{!r} does not start with py_".format(key)) which breaks your args[0] usage but is a much more descriptive message and has no defined way to provide the key as an attribute.

It's basically "explicit is better than implicit" since there is no API guarantee that the first thing in 'args' for KeyError will in actuality be the key.

mdickinson commented 11 years ago

+1. I recently chastised a colleague for doing "raise KeyError(long_message)" instead of "raise KeyError(missing_item)". When I went to the standard library to support my POV, I found (to my chagrin) a big mix of the two styles.

>>> from collections import ChainMap
>>> d = ChainMap({}, {})
>>> try:
...     d.pop('not there')
... except KeyError as e:
...     key, = e.args
... 
>>> key
"Key not found in the first mapping: 'not there'"
797ba50b-2abd-43a3-b527-4817fd770d7c commented 4 years ago

Hi all,

As a Python developer, I encountered lots of blurry exception messages in the product logs such as: \<time and date> \<process name> \<log severity> Failed to do something. Exception: 'some key'

I believe printing the key name without explaining the exception itself is bad (explicit is better than implicit).

Thus, after forking the repository, I added a short explanation about the Exception. Now, it looks like:

>>> try:
...     {}['some key']
... except Exception as e:
...     print(e)
... 
Missing key: some key

I'm a newbie in a contribution to CPython, so please let me know if the idea behind my commit is good. If not, please explain how can I improve my commit so it'll be pulled later to the main branch.

https://github.com/zingero/cpython/commit/8c9e5d9e16296cee1f3ec05638dd6989dae8b811

Regards, Orian.

45cf1e81-2e2b-4848-9af2-e47b9c53822f commented 4 years ago

I agree with Mark. I will be useful to provide ability to specify custom long message for error. And logically it would be better to do not allow users change key field in error so easy.

b786cac0-71df-4c35-a58b-012a049d8b3d commented 3 years ago

Orian: your patch formats the error message but the original suggested solution is to store the missing key in a new attribute.

I don't know if you go in the good direction.

Adding an attribute is also suggested by issue bpo-614557.

b786cac0-71df-4c35-a58b-012a049d8b3d commented 3 years ago

I'm interested by such feature.

I see examples of versions of the message provided by KeyError:

It explains why there is a difference in the messages in KeyError (as said in previous messages).

PyErr_SetString(), PyErr_Format(), PyErr_FormatV() (implemented in Python/errors.c) don't have a parameter to set the missing key. So I think it would be easier to set the missing attribute before calling thoses functions.

According to [3], the C PyExc_KeyError matches the Python KeyError exception.

So I think to:

Do you think such strategy is doable? What do you think about it? Is it the way you think about it? If not, do you have some hint?

I already made some minor patches to cpython but only in the Python part, never in C one.

1: https://github.com/python/cpython/blob/master/Modules/unicodedata.c#L1398 2: https://github.com/python/cpython/blob/master/Python/hamt.c#L2767 3: https://docs.python.org/3/c-api/exceptions.html?highlight=pyerr_format#standard-exceptions 4: https://github.com/python/cpython/blob/master/Objects/exceptions.c#L1569