Skip to content

RcParams is fundamentally broken #12576

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.

Sign up for GitHub

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

Jump to bottom
Closed
timhoffm opened this issue Oct 20, 2018 · 7 comments
Closed

RcParams is fundamentally broken #12576

timhoffm opened this issue Oct 20, 2018 · 7 comments
Milestone
v3.0.2

Comments

@timhoffm
Copy link
Member

timhoffm commented Oct 20, 2018 •
edited
Loading

Bug report

Anyone tried:

>>> import matplotlib
>>> len(matplotlib.rcParams)
0

A great example of shooting yourself in the foot with multiple inheritance. Essentially, we do

class RcParams(MutableMapping, dict):
    def __getitem__(self, key):
        return dict.__getitem__(self, key)

    def __setitem__(self, key, val):
        return dict.__setitem__(self, key, val)

but do not reimplement __len__. You can work out the consequences yourself.

Proposed solution

Do not inherit RcParams from dict, and use a private data dict instead.
@timhoffm timhoffm mentioned this issue Oct 20, 2018
Closed
@timhoffm
Copy link
Member Author

Fun fact: This results in rcParams.clear() to not have any effect.

@tacaswell tacaswell added this to the v3.1 milestone Oct 23, 2018
@tacaswell
Copy link
Member

Could we just implement __len__ as a quicker fix? I am curious of this ever worked....

@timhoffm
Copy link
Member Author

Yes, implementing __len__ is probably a workaround, and I'm fine with that. Don't have time to go into the details in the next days.

I assume that this worked when RcParams was just inheriting from dict (not tested).

Anyway, in the long term, we should aim at just inheriting from MutableMapping.

@anntzer
Copy link
Contributor

anntzer commented Oct 25, 2018

Could we just implement len as a quicker fix? I am curious of this ever worked....

I think it's a bug in CPython: https://bugs.python.org/issue35063

@timhoffm
Copy link
Member Author

Not having fully read the bug, but implementing __len__ would work around this, right?

@anntzer
Copy link
Contributor

anntzer commented Oct 25, 2018

Yes.
The bug is that RcParams() should have raised an exception given than __len__ is a not-overridden abstractmethod.

@timhoffm timhoffm mentioned this issue Oct 25, 2018
Merged
@timhoffm
Copy link
Member Author

timhoffm commented Nov 4, 2018

Closing as the current problem is fixed by #12630.

Still, I would like to get rid of the dict-inheritance in the the long run (#12577).

@timhoffm timhoffm closed this as completed Nov 4, 2018
@QuLogic QuLogic modified the milestones: v3.1, v3.0.x Nov 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.2
Development

Successfully merging a pull request may close this issue.

RcParams should not inherit from dict timhoffm/matplotlib
4 participants
@tacaswell @QuLogic @anntzer @timhoffm