-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Updated argument to mpl.ticker.LogFormatter.label_minor
#6264
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
Just closed and reopened to restart Appveyor. There were no actual build errors. |
498deed
to
f32bd5e
Compare
f32bd5e
to
61ad51b
Compare
Squawk |
Bump. |
I don't see the point in changing this, but if you are going to do this, can you replace the camel case with something more pythonic? (to for example |
61ad51b
to
869a776
Compare
You are absolutely right. I made both the camel->_ change and updated the docs slightly. Thanks for the explanation. Hopefully this change is no longer a controversial one. I just remembered that the whole point of the change was that the sense of the argument is opposite to the sense of the method name, but at this point I don't care much any more as long as the docs are clear enough. |
869a776
to
7ab73cc
Compare
Minor clarification to docs as well.
7ab73cc
to
df5f99b
Compare
I am overall in favor of that change, but we will need to maintain backward compatibility for at least two version. That means adding back the previous argument name, and checking whether the user sets it manually or not, and raising a deprecation message if the user does. |
@efiring and @tacaswell - can you comment and approve or decline this API change? |
@@ -829,13 +829,13 @@ class LogFormatter(Formatter): | |||
""" | |||
Format values for log axis. | |||
""" | |||
def __init__(self, base=10.0, labelOnlyBase=True): | |||
def __init__(self, base=10.0, label_only_base=True): |
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 this fix has to hold off until we have python3 only and can do
def __init__(self, base, label_only_base, * labelOnlyBase=none)
and then deprecate it
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 guess we can do this for both with
def __init__(self, base, label_only_base=True, **kwargs)
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.
The easiest way to do this is …, label_only_base=True, labelOnlyBase=None
and check whether labelOnlyBase was provided to deprecate it.
""" | ||
self._base = base + 0.0 | ||
self.labelOnlyBase = labelOnlyBase | ||
self.label_only_base = label_only_base |
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.
This is also a public API break which should be patched over with a property, but should also wait for changing the parameter name in the __init__
I am not convinced that the work to maintain back compatibility and then the deprecation cycle is worth changing the name. weak 👎 . |
@NelleV. There has been a lot going on, so I did not expect much attention to this PR. I am getting the sense that this may be too much work for a really trivial change that could probably be solved by documentation. Just for the fun of it, I will add the deprecation in to see if you like it. |
I am not in favor of this. If we are going to have a CamelCase cleanup in ticker, it should be done all at once rather than piecemeal. (Whether it will ever be worth doing is debatable.) |
There seems to be a consensus that this patch is not worth the trouble. I am closing the PR. |
I think under the circumstances that is the only thing to do. Thanks for taking care of it for me. |
@madphysicist Just to re-iterate @NelleV , thank you for working on this. Saying no is one of the hard things about maintaining mpl 😞 . |
This is PR is opened in response to #6253 (comment). It reverses the implied meaning of the input parameter.