Remove (some) features deprecated in mpl2.2#12245
Conversation
| ``GridSpecFromSubplotSpec.get_subplot_params``, | ||
| - svgfont support (in :rc:`svg.fonttype`), | ||
| - passing 'box-forced' to `axes.Axes.set_adjustable`, | ||
| - support for the strings 'on'/'true'/'off'/'false' to mean True/False. |
There was a problem hiding this comment.
I think the methods this affects should be listed here
dstansby
left a comment
dstansby
left a comment
There was a problem hiding this comment.
Having just spend ages trying to work out how to replace deprecated code in another project, I think all the deprecations here should have clear alternatives in the api_changes file where appropriate; I'm happy to work on this and push to this branch @anntzer?
|
Go for it. |
| orientation='vertical', rwidth=None, log=False, | ||
| color=None, label=None, stacked=False, normed=None, | ||
| **kwargs): | ||
| color=None, label=None, stacked=False, **kwargs): |
There was a problem hiding this comment.
Will this now just silently suck up normed into **kwargs, and not do anything? If so I think this removal needs a bit more thought (ie. maybe 1 or 2 releases that error when normed is passed).
There was a problem hiding this comment.
I unremoved the normed kwargs. Feel free to handle its removal as you see fit.
|
I took the liberty to push more API-change notes. Each change should have an alternative given or stated that there is no alternative. I'm ok with all the changes, but of course, cannot approve formally myself anymore because I've now code in this. |
|
Rebased. @dstansby can you re-review? Edit: Hmmmm sorry failing the tests now. |
| The update parameter control which parameter of the legend changes | ||
| when dragged. If update is "loc", the *loc* parameter of the legend | ||
| is changed. If "bbox", the *bbox_to_anchor* parameter is changed. | ||
| """ |
There was a problem hiding this comment.
Just a comment here... this docstring was never updated to note the deprecation, which means one would not know of the deprecation until they actually use the method. That seems sub-optimal.
There was a problem hiding this comment.
But if someone is not actually using the method, how are they bothered by the deprecation and removal?
| ``backend_bases.FigureManagerBase.show_popup`` (this never did anything) | ||
| - ``backend_wx.SubplotToolWx`` (no replacement) | ||
| - ``backend_wx.Toolbar`` (use ``backend_wx.NavigationToolbar2Wx`` instead) | ||
| - ``cbook.align_iterators`` (no replacment) |
There was a problem hiding this comment.
Perhaps say "use pandas instead" or something?
There was a problem hiding this comment.
If you have a specific replacement in mind, please push it (or I can do it), but otherwise that seems a bit vague (at least I don't know what replacement you're thinking about).
|
I am not particularly enthused by the number of "no replacement" we are putting in the documentation. I feel like we need to at least throw a line to users for similar functionality, or a hint towards how to implement the missing feature themselves. In addition, for future deprecations, I think we might want to have this sort of guidance documented at the time of deprecation, not at the time of removal. That way, people can know how to update their code at a normal cadence, rather than later (not saying that will always happen, but one can hope!) |
They can copy-paste the code from an old version of matplotlib. |
|
test failures should be fixed now. |
dstansby
dismissed
their stale review
Don't have time to look at this, but my earlier concerns have been fixed
dstansby
added
status: needs rebase
Release critical
|
rebased |
anntzer
added
Release critical
|
Looks good to me, I just added a few words to the note about svg support to make it clearer 👍 |
|
Needs rebase to unbreak the doc builds (c.f. #13080). |
|
done |
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
I'm bold and approve even though I pushed a commit. But that commit only documented some alternatives and added removed one now-unnecessary argument case.
|
Going to merge on the basis of two reviews and it's probably better to get this in sooner rather than later so people using the |
4 participants
PR Summary
@dstansby gets to do mlab in #12165 :)
PR Checklist