X Tutup
The Wayback Machine - https://web.archive.org/web/20260312135744/https://github.com/python/cpython/pull/28892
Skip to content

Add MappingView.mapping for parity with dictview#28892

Closed
jab wants to merge 1 commit intopython:mainfrom
jab:mapping
Closed

Add MappingView.mapping for parity with dictview#28892
jab wants to merge 1 commit intopython:mainfrom
jab:mapping

Conversation

@jab
Copy link
Contributor

@jab jab commented Oct 11, 2021 •
edited
Loading

As of gh-20749, built-in dictviews now have a mapping attribute.

This PR adds the same to collections.abc.MappingView, so that non-builtin mappings can get feature parity, not to mention a consistent interface, just by inheriting from collections.abc.Mapping (as many already do), without having to add any boilerplate code of their own.

From the associated original issue bpo-40890, it looks like this was never discussed or considered. Perhaps this would have been added at the same time as gh-20749 if it had been part of the original discussion. Submitting this (currently draft) 3-line PR as a starting point for further discussion. The changes in this PR already make the following code work:

# demo.txt

>>> from collections.abc import Mapping

>>> class MyMapping(Mapping):
...     def __init__(self, source: Mapping):
...         self._source = source
... 
...     def __iter__(self):
...         return iter(self._source)
... 
...     def __len__(self):
...         return len(self._source)
... 
...     def __getitem__(self, key):
...         return self._source[key]
... 
...     def __repr__(self):
...         return f'{self.__class__.__name__}({self._source})'


>>> d = dict(one=1, two=2)
>>> d.keys().mapping
mappingproxy({'one': 1, 'two': 2})

>>> m = MyMapping(d)
>>> m.keys().mapping
mappingproxy(MyMapping({'one': 1, 'two': 2}))
$ ./python.exe -VV
Python 3.11.0a1+ (heads/main-dirty:7103356455, Oct 11 2021, 17:28:12) [Clang 12.0.0 (clang-1200.0.32.29)]

$ ./python.exe -m doctest demo.txt  # no output + exits zero => it works

If it makes sense to pursue this, I can create a proper bpo issue and put the finishing touches on this PR.

Context: I noticed this while reviewing the changes in Python 3.10 to see if they called for any corresponding changes to my bidict library, which I believe is one of several libraries that would benefit from fixing this issue.

/cc @rhettinger @sweeneyde et al.

@jab jab marked this pull request as ready for review October 11, 2021 23:31
@jab jab requested a review from rhettinger as a code owner October 11, 2021 23:31
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 12, 2021 •
edited by bedevere-bot
Loading

Please open a new issue. bpo-40890 is closed long time ago, and it was about feature added in 3.10.

Why not use property?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 12, 2021

Also, new feature needs tests, documentation, etc.

@jab
Copy link
Contributor Author

jab commented Oct 12, 2021

Sure, just want to hear if @rhettinger agrees with this before I put any more time into it.

@rhettinger
Copy link
Contributor

rhettinger commented Oct 17, 2021

This is a nice thought but can't go forward. The ABCs specify a minimum API requirement, not just for derived classes but also for registered classes. Accordingly, we cannot ever expand the API because formerly compliant APIs would stop being compliant.

In general, concrete APIs such as dictview are allowed to add extra capabilities, but the ABCs themselves cannot expand once they are published.

If an extension is truly needed, then we could offer an new ABC with the extra capability. It the case of the mapping attribute, it likely isn't worth it.

@rhettinger rhettinger closed this Oct 17, 2021
@jab
Copy link
Contributor Author

jab commented Oct 18, 2021 •
edited
Loading

I didn’t think this changes the contract for any class registered as a MappingView. Only methods marked @abstractmethod are required to be provided by registered classes, and this adds no new abstract methods. In other words, an already-registered MappingView would still be compliant, even if it didn’t provide the optional .mapping attribute. What am I missing?

@jab
Copy link
Contributor Author

jab commented Oct 18, 2021

Going back to the https://docs.python.org/3/library/collections.abc.html docs, I guess you’re considering the new .mapping attribute proposed here to be similar to a mixin method of the ABC, i.e. not enforced by the ABC, but still should be provided by registered classes, is that right?

@jab
Copy link
Contributor Author

jab commented Oct 29, 2021

@rhettinger, your concern for the .mapping attribute's compatibility with existing code is even more justified than I realized, and ironically, I found a new compatibility issue with the existing implementation! Please see https://bugs.python.org/issue45670.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jab @serhiy-storchaka @rhettinger @the-knights-who-say-ni @bedevere-bot
X Tutup