-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Revert change of parameter name in annotate() #12383
Conversation
ftr there's #7966 re: signature-overloaded functions, which can easily be adapted to handle deprecations in signature changes. |
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 doing an additional deprecation cycle for this specific un-change is really not worth it.
6021a60
to
edaeca4
Compare
I think this is a bug fix for |
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.
Looks fine; no need for anything more than this.
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 |
PR Summary
Change the parameter name
annotate(text, ...)
back toannotate(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
ortitle
instead ofs
. 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 adds
back in as a fallback (including changing the originalAnnotation
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