Skip to content

Deprecate unused parameters #16096

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

Merged
merged 1 commit into from
Jan 6, 2020
Merged

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Jan 4, 2020

PR Summary

Unused parameters to public functions.

@@ -2693,6 +2693,10 @@ def set_message(self, s):

def back(self, *args):
"""Move back up the view lim stack."""
if args:
Copy link
Contributor

@anntzer anntzer Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16063 gives you @_delete_parameter(version, "args") and @_delete_parameter(version, "kwargs").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that gets in first, I'll adapt, but wouldn't wait for this. BTW, I've approved #16063.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also steal the implementation of _delete_parameter if you want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave that to you 😄

@timhoffm timhoffm force-pushed the depecate-unused branch 2 times, most recently from bb7db6d to 2a22dfc Compare January 4, 2020 21:37
@@ -53,7 +53,8 @@ class Tick(martist.Artist):
The right/top tick label.

"""
def __init__(self, axes, loc, label,
@cbook._delete_parameter("3.3", "label")
def __init__(self, axes, loc, label=None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit strange because the Tick now takes a bunch of kwargs which basically get forwarded to the label Text objects (labelsize, labelcolor, labelrotation) but can't set the text itself? (IOW should we make the label kwarg actually work instead?) Alternatively should these labelfoos also get deprecated?

Copy link
Member Author

@timhoffm timhoffm Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. The label text is not really in the control of the tick but in the control of the Locator/Formatter. All usages in our codebase just passed an empty string. So making label work seems not too desirable.

I think labelfoos are currently used for creating additional ticks with the right properties after set_tick_params() was used. Would have to check for that.

Generally, I'd rather make the Tick API smaller than larger because it's a bit performance critical; and we may want to migrate to somthing like a TickCollection at some point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly agree with making it smaller rather than larger, but would rather get rid of all label related args together if possible. Won't block over that though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can look into that, but would rather do in a separate PR. I have the itch that everything will break down if I remove the labelfoos. And sorting that out in a small clean PR is simpler. Also it's easier to revert if we find out this was a bad idea.

@timhoffm timhoffm force-pushed the depecate-unused branch 2 times, most recently from 4729c7b to 7f46eef Compare January 5, 2020 11:41
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postci

@jklymak jklymak merged commit 45cfd87 into matplotlib:master Jan 6, 2020
@timhoffm timhoffm deleted the depecate-unused branch January 6, 2020 19:50
yuzie007 added a commit to yuzie007/mpltern that referenced this pull request Jul 18, 2020
- matplotlib/matplotlib#16096
- matplotlib/matplotlib@SHA d8f815d36b204af86ea39734588aa5f771fbc45f
yuzie007 added a commit to yuzie007/mpltern that referenced this pull request Jul 18, 2020
- matplotlib/matplotlib#16096
- matplotlib/matplotlib@SHA: d8f815d36b204af86ea39734588aa5f771fbc45f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants