Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposal: Implement
RcParams
using ChainMap and removedict
inheritance #25617New 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
base: main
Are you sure you want to change the base?
Proposal: Implement
RcParams
using ChainMap and removedict
inheritance #25617Changes from all commits
ab1ea99
5177508
45d4743
133f2af
b7f4fb5
d135342
47b48ee
9a08544
9a0ad03
7a11e81
4f9d8b6
54a1a11
c101fec
67735b0
7e4022a
c23cb29
0374f46
dd99c1d
aa282d0
b1754a2
faed9ba
dfbb73f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 775 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L774-L775
Check warning on line 777 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L777
Check warning on line 781 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L780-L781
Check warning on line 783 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L783
Check warning on line 798 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L798
Check warning on line 804 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L801-L804
Check warning on line 832 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L832
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On one hand,
clear
is current a no-op becausepopitem
raisesKeyError
which makes the default implementation fromMutableMapping
conclude that there is nothing to do and it is cleared despite doing nothing.This is part of the public API of the
MutableMapping
API so I don't think we can (should) actually deprecate this.We should either make sure the note says "we are going to make this error in the future" or just warn "this is not doing what you think it is doing".
Check warning on line 843 in lib/matplotlib/__init__.py
lib/matplotlib/__init__.py#L843
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current
setdefault
behavior (side-effect of__setitem__
not accepting new keys): Return the value if key is valid, otherwise raise. We should keep this behavior for compatibility.Note that this behavoir is identical to
rcParams[key]
. Optional: Deprecatesetdefault
and recommendrcParams[key]
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just return the value (maybe catching the
KeyError
on misses to match the current error message exactly) and explicitly handle the one case we use this internally .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be more specific about the type of the values?
e.g.
tuple[str, ...]
(this is how you say homogeneous tuple of str, indeterminate length)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks! I wasn't sure how to explain indeterminate length so kept it as just a tuple. I'll update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually inherited from MutableMapping, but doesn't hurt to put it here more explicitly...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added these mainly because we overwrite these functions. So, just to be consistent with having stubs for the functions implemented..
Check warning on line 2481 in lib/matplotlib/pyplot.py
lib/matplotlib/pyplot.py#L2481