-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Simplify the rcparams deprecation machinery. #15329
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.
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
Conversation
7b27bc4
to
f0bec85
Compare
5207deb
to
4cc64ce
Compare
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'm 👎. Usage of dict.__getitem__(rcParams, par)
for working around deprecation warnings is really a crude hack. I don't want to see this all over the code. Related: #12577.
Right now read access on deprecated rcParams does not warn, which is quite clearly a bug. I think it is reasonable to say that |
I'd go with |
4cc64ce
to
0476889
Compare
Sure, added such a helper. |
Actually I'm not sure if it's wise to throw away the parameter mapping capabilities. While we don't have any pararmeters in there right know, I can imagine that we may want to rename parameters in the future. |
We can always restore it if the need shows up, but right now it's quite a bit of complexity in the rcParams implementation for no use. |
Hm, but fiddling that complexity back in is cumbersome as well and the present code doesn't hurt too much (or do you plan larger work on that part?). I'd llke to hear thoughts of the other devs on this. |
I don't plan to fiddle too much with it, but the complexity was such that the basic goal of this code (warning on access to deprecated rcParams, e.g. |
Just have a single kind of deprecated rcParams, which emit a DeprecationWarning on get and on set. ... which caught the fact that we were previously *not* warning on access to text.latex.unicode, now we do but need to avoid triggering the warnings internally.
I am inclined to decline this PR. |
At least the problem that _deprecated_remains_as_none entries (e.g. text.latex.unicode -- now removed -- which is mentioned in the original post) did not trigger a warning on getitem remains. |
I think that this is a compelling position. We do it rarely enough and simply writing the code for it would likely be simpler and easier to read. |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
@timhoffm Thoughts about just killing off the rc entire deprecation machinery (#15329 (comment)) and writing ad hoc code as needed? |
I support throwing out the whole machinery and write the code as needed. |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
Let's get rid of the whole deprecation machinery. Replaced by #29562. |
Just have a single kind of deprecated rcParams, which emit a
DeprecationWarning on get and on set.
... which caught the fact that we were previously not warning on
access to text.latex.unicode, now we do but need to avoid triggering the
warnings internally.
PR Summary
PR Checklist