Data access API for rcParams#24730
Conversation
|
Thanks for the ping. The replacement seems OK, but perhaps you could just make the API |
timhoffm
force-pushed
the
rcparams-data-api
branch
2 times, most recently
from
e8ff5bb to
888811c
Compare
oscargus
left a comment
oscargus
left a comment
There was a problem hiding this comment.
Some minor comments. Looks good, but I guess other people have better insights into what is actually wanted.
| @@ -605,7 +605,7 @@ def gen_candidates(): | |||
| ) | |||
| class RcParams(MutableMapping, dict): | |||
There was a problem hiding this comment.
I may not fully get the scope here, but I take it that one reason to do this is that we should not subclass from dict later on? Or is the whole idea to provide a stable API for this, but actually still subclass?
There was a problem hiding this comment.
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 via rcParams; or we have a new completely free to design config object and rcParams will loose all state and only be an adapter to that new config object. Either way, being bound to a dict subclass would be cumbersome.
timhoffm
force-pushed
the
rcparams-data-api
branch
from
888811c to
986ea7c
Compare
|
I guess I don't fully understand the need for this change, even after reading the comments before. Is there some reason |
|
Note CI didn't run for this. |
|
Discussed on call, consensus is that while |
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
Sorry, I didn't grok that this was calling dict.__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 to dict. Added a few more clarifying phrases to the note that would have helped me.
timhoffm
force-pushed
the
rcparams-data-api
branch
from
cc2f863 to
e29ab5b
Compare
timhoffm
force-pushed
the
rcparams-data-api
branch
from
c6425b1 to
338ee3d
Compare
story645
left a comment
story645
left a comment
There was a problem hiding this comment.
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.
timhoffm
force-pushed
the
rcparams-data-api
branch
from
338ee3d to
11df707
Compare
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 dict subclass. The new data access API gets rid of this dependence and thus opens 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 allows other future restructings like introducing a new configuration management and 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
the
rcparams-data-api
branch
from
11df707 to
ecf156c
Compare
8 participants
PR Summary
Replaces (first step of) #12577, see in particular #12577 (comment).
This provides a defined API for accessing rcParams via
rcParams._data[key]while circumventing any validation logic happening inrcParams[key].Before, direct data access was realized through
dict.__getitem__(rcParams, key)/dict.__setitem__(rcParams, key, val), which depends on the implementation detail ofrcParamsbeing 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 the
MutableMappinginterface forrcParamsin the future. This allows other future restructings like introducing a new configuration management and changingrcParamsinto a backward-compatible adapter.Ping @anntzer who will be affected by this change in mplcairo #12577 (comment)