Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

odict_items and dict_items' repr's don't match OrderedDict's and dict's #113802

Closed
jab opened this issue Jan 8, 2024 · 4 comments
Closed

odict_items and dict_items' repr's don't match OrderedDict's and dict's #113802

jab opened this issue Jan 8, 2024 · 4 comments
Labels
pending The issue will be closed if no feedback is provided type-feature A feature request or enhancement

Comments

@jab
Copy link
Contributor

jab commented Jan 8, 2024

Bug report

Bug description:

As of #101446, OrderedDict's repr now looks like dict's, but their ItemsViews were not updated to match.

python3.12
Python 3.12.0 (main, Oct  2 2023, 12:03:24) [Clang 16.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from collections import OrderedDict
>>> OrderedDict({1: -1, 2: -2})  # no longer looks like a list of pairs
OrderedDict({1: -1, 2: -2})
>>> OrderedDict({1: -1, 2: -2}).items()  # still looks like a list of pairs
odict_items([(1, -1), (2, -2)])
>>> {1: -1, 2: -2}.items()  # ditto
dict_items([(1, -1), (2, -2)])

CPython versions tested on:

3.12

Operating systems tested on:

No response

@jab jab added the type-bug An unexpected behavior, bug, or error label Jan 8, 2024
@serhiy-storchaka
Copy link
Member

And what is wrong with it? It is consistent with keys view and values view. Iterating an items view emits pairs.

@Eclips4 Eclips4 added the pending The issue will be closed if no feedback is provided label Jan 8, 2024
@jab
Copy link
Contributor Author

jab commented Jan 8, 2024

Thanks for taking a look. I wasn't sure if this was a deliberate decision, or if maybe just no one thought about it at the time #101446 was introduced (I didn't see any discussion of this there or in the associated PR). If you feel it's worth it, please ping Raymond or anyone else involved in the previous changes to weigh in here.

Regarding your consistency point:

Since ItemsViews contain items (not single values like Keys- or ValuesViews), is it better to be consistent with dict's repr, which also displays items as opposed to single values?

Here's what that would look like in context:

>>> {1: 1, 2: 2}
{1: 1, 2: 2}

>>> {1: 1, 2: 2}.items()
dict_items({1: 1, 2, 2})

>>> OrderedDict({1: 1, 2: 2})
OrderedDict({1: 1, 2: 2})

>>> OrderedDict({1: 1, 2: 2}).items()
odict_items({1: 1, 2: 2})


>>> {1: 1, 2: 2}.keys()
dict_keys([1, 2])

>>> OrderedDict({1: 1, 2: 2}).keys()
odict_keys([1, 2])

This would also provide the nice property that all view repr's would consistently have only 2 levels of nested brackets, instead of 3 in the current case of only the items views.

Independent of any consistency-based rationale, more layers of nested brackets are noisier and not as easy to read compared to fewer layers, so that's another reason to consider this proposed change.

@serhiy-storchaka
Copy link
Member

On one hand, the repr will be smaller and with less nesting level.

On other hand, it will be less obvious that dict_items represents a collection of pairs, and it repr will be more distinct from reprs of dict_keys and dict_values. It may be worse for education.

It is a very minor issue and I have no strong opinion. cc @rhettinger

@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jan 10, 2024
@rhettinger
Copy link
Contributor

It is a very minor issue and I have no strong opinion. cc @rhettinger

I'm think the status quo is reasonable and we should leave it as-is.

It was a perfectly reasonable choice for OrderedDict() and its views to follow what dict() does. Afterall, `OrderedDict is a dict subclass.

In contrast, the ItemsView ABCs can't make any assumptions about the underlying mapping, so it is reasonable to just show it's __repr__ so that a programmer can see what was wrapped. The underlying mapping might be virtual, case-insensitive, or have some other interesting behavior that wouldn't be well expressed by only showing a list of pairs.

@rhettinger rhettinger closed this as not planned Won't fix, can't repro, duplicate, stale Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending The issue will be closed if no feedback is provided type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants