Skip to content

Revert change of parameter name in annotate() #12383

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
Oct 4, 2018

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Oct 3, 2018

PR Summary

Change the parameter name annotate(text, ...) back to annotate(s, ...) (was introduced in #10830).

Fixes #12325.

Not sure if it's ok to go back straight. IMO, it's rather uncommon to pass this param as kwarg. But technically, that's an API change again. If people have adapted for s -> text in 3.0, they would have to adapt back again.

Possible alternative

Generally, I'd rather have descriptive argument names such as text or title instead of s. However, I'm learning to be more careful with API changes and these things would have to go through a deprecation mechanism with a transition phase of allowing both. If there is a general approval to migrate parameter names in such a way, I can apply the mechanism here and just add s back in as a fallback (including changing the original Annotation class as well.

Note: Thinking about it, a transition mechanism is technically possible, however a bit complicated. We would have to support:

  • annotate('text', xy)
  • annotate(s='text', xy=xy)
  • annotate(text='text', xy=xy)

which would require

def annotate(self, text=None, xy=None, ..., s=None):
"""
annotate(self, text, xy, ...)
"""
# merge text and s
# check that xy is not None

@anntzer
Copy link
Contributor

anntzer commented Oct 3, 2018

ftr there's #7966 re: signature-overloaded functions, which can easily be adapted to handle deprecations in signature changes.

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.

I think doing an additional deprecation cycle for this specific un-change is really not worth it.

@timhoffm timhoffm force-pushed the revert-annotate-parameter-name branch from 6021a60 to edaeca4 Compare October 3, 2018 10:50
@jklymak jklymak added this to the v3.0.x milestone Oct 3, 2018
@jklymak
Copy link
Member

jklymak commented Oct 3, 2018

I think this is a bug fix for 3.0.x. But @tacaswell, @dopplershift, or @efiring should weigh in on whether this reversion needs to be deprecated.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

Looks fine; no need for anything more than this.

@efiring
Copy link
Member

efiring commented Oct 4, 2018

As a general comment, I think that sticking with "s" is the way to go. It stands for "string", contrasts adequately with "xy", avoids confusion or duplication with the text function and method, and is usually used as a positional argument anyway. And it has been there since the beginning, hasn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants