Before, direct data access was realized throughdict.__getitem__(rcParams, key) /dict.__setitem__(rcParams, key, val), which depends on the implementation detail ofrcParams being a dict subclass. The new data access API gets rid of this dependency and thus opens up a way to later move away from dict subclassing.

We want to move away from dict subclassing and only guarantee theMutableMapping interface forrcParams in the future. This allows other future restructings like introducing a new configuration management and changingrcParams into a backward-compatible adapter.

Ping@anntzer who will be affected by this change in mplcairo#12577 (comment)

timhoffm added API: changes Maintenance labels

Dec 14, 2022

timhoffm added this to thev3.7.0 milestone

Dec 14, 2022

Copy link

Contributor

anntzer commentedDec 14, 2022•
edited
Loading

Thanks for the ping. The replacement seems OK, but perhaps you could just make the APIrcParams._get(key) andrcParams._set(key, value) which avoids introducing another private helper class and can just be done with plain methods? (Also youcould mark the methods with:meta public: so that they show up in the rendered docs, seehttps://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html)

timhoffm force-pushed thercparams-data-api branch 2 times, most recently frome8ff5bb to888811cCompare

December 15, 2022 10:37

oscargus reviewed

Dec 15, 2022

View reviewed changes

Copy link

Member

oscargus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some minor comments. Looks good, but I guess other people have better insights into what is actually wanted.

doc/api/next_api_changes/deprecations/24730-TH.rst OutdatedShow resolvedHide resolved

lib/matplotlib/__init__.py

		@@ -605,7 +605,7 @@ def gen_candidates():
		)
		class RcParams(MutableMapping, dict):

Copy link

Member

oscargusDec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I may not fully get the scope here, but I take it that one reason to do this is that we should not subclass fromdict later on? Or is the whole idea to provide a stable API for this, but actually still subclass?

Copy link

MemberAuthor

timhoffmDec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We want to get rid of dict subclassing eventually. Using a dict subclass dictates the internal data model. We likely don't want this in the future when we're remodelling the config data structure. Either the new structure will be a 100% API compatible drop in and the config object will be available viarcParams; or we have a new completely free to designconfig object andrcParams will loose all state and only be an adapter to that newconfig object. Either way, being bound to a dict subclass would be cumbersome.

timhoffm force-pushed thercparams-data-api branch from888811c to986ea7cCompare

December 15, 2022 12:15

tacaswell approved these changes

Dec 15, 2022

View reviewed changes

Copy link

Member

jklymak commentedDec 15, 2022

I guess I don't fully understand the need for this change, even after reading the comments before. Is there some reason_set and_get wouldn't just be aliased to__setitem__ and__getitem__ in the new class, even without subclassingdict?

Copy link

Member

jklymak commentedDec 15, 2022

Note CI didn't run for this.

Copy link

Member

tacaswell commentedDec 15, 2022

Discussed on call, consensus is that while_get and_set are good ideas, we should refrain from making any major changes to the current implementation until we have a better idea what we want to replace it with.

jklymak reviewed

Dec 15, 2022

View reviewed changes

Copy link

Member

jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sorry, I didn't grok that this was callingdict.__get_item__, and hence didn't understand the point of this. I now understand and think it is a reasonable thing to add direct private access without hacking up an inheritance level todict. Added a few more clarifying phrases to the note that would have helped me.

doc/api/next_api_changes/deprecations/24730-TH.rst OutdatedShow resolvedHide resolved

lib/matplotlib/pyplot.py OutdatedShow resolvedHide resolved

timhoffm force-pushed thercparams-data-api branch fromcc2f863 toe29ab5bCompare

December 17, 2022 00:18

QuLogic reviewed

Dec 17, 2022

View reviewed changes

doc/api/next_api_changes/deprecations/24730-TH.rst OutdatedShow resolvedHide resolved

timhoffm force-pushed thercparams-data-api branch fromc6425b1 to338ee3dCompare

December 17, 2022 16:52

story645 reviewed

Dec 22, 2022

View reviewed changes

Copy link

Member

story645 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Have no objection on adding a shim/translation layer, especially if it's kept private 'til an API gets settled on - mostly just documentation nits about how this is communicating that this is a "don't touch, for contributors only" API.

doc/api/next_api_changes/deprecations/24730-TH.rst OutdatedShow resolvedHide resolved

lib/matplotlib/__init__.py OutdatedShow resolvedHide resolved

lib/matplotlib/__init__.pyShow resolvedHide resolved

timhoffm force-pushed thercparams-data-api branch from338ee3d to11df707Compare

December 22, 2022 22:16

Data access API for rcParams

ecf156c

This provides a defined API for accessing rcParams via`rcParams._data[key]`while circumventing any validation logic happening in`rcParams[key]`.Before, direct data access was realized through`dict.__getitem(rcParams, key)` / `dict.__setitem(rcParams, key, val)`,which depends on the implementation detail of `rcParams` being a dictsubclass. The new data access API gets rid of this dependence and thusopens up a way to later move away from dict subclassing.We want to move away from dict subclassing and only guarantee the`MutableMapping` interface for `rcParams` in the future. This allowsother future restructings like introducing a new configuration managementand changing `rcParams` into a backward-compatible adapter.Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>Co-authored-by: Jody Klymak <jklymak@gmail.com>Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>Co-authored-by: story645 <story645@gmail.com>

timhoffm force-pushed thercparams-data-api branch from11df707 toecf156cCompare

December 22, 2022 22:24

story645 approved these changes

Dec 22, 2022

View reviewed changes