8000 Deprecate setting Axis.major.locator to non-Locator; idem for Formatters by anntzer · Pull Request #13851 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Deprecate setting Axis.major.locator to non-Locator; idem for Formatters #13851

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.

Sign up for GitHub

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
Apr 21, 2019
Merged

Deprecate setting Axis.major.locator to non-Locator; idem for Formatters #13851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions doc/api/next_api_changes/2019-04-02-AL.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Deprecations
````````````

Setting ``Axis.major.locator``, ``Axis.minor.locator``, ``Axis.major.formatter``
or ``Axis.minor.formatter`` to an object that is not a subclass of `Locator` or
`Formatter` (respectively) is deprecated. Note that these attributes should
usually be set using `Axis.set_major_locator`, `Axis.set_minor_locator`, etc.
Copy link
Member
@timhoffm timhoffm Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Should they be set at all directly on the ticker? I don't think so at least for locators, because Axis.set_major_locator ensures that the formatter knows the locator, which is not guaranteed when setting Axis.major.locator.

So maybe add _set_locator() and _set_formatter() for the internal API. And deprecate property setter for all inputs.

Copy link
Contributor Author
@anntzer anntzer Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Well, you get to argue about https://github.com/matplotlib/matplotlib/pull/13826/files#diff-52127080e2783464f529df684d82d476R1319 with @tacaswell then :)

(Note that we could move the call to _set_locator in locator.setter, though.)

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the point. The linked diff does not assign to axis.major.locator or axis.major.formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does getattr(self.minor.locator, "remove_overlaps", True) instead of the simpler self.minor.locator.remove_overlaps (note that remove_overlaps is in the base Locator class) specifically to handle the case of a locator not inheriting from Locator.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see that we jump through some hoops to support non-Locator instances as locator.

But _set_locator() would be living in TickHelper, which hopefully nobody has replaced. AFAICS non-Locator objects would still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't get what you're saying about TickHelper. Did you mean Ticker (which is not the same)?

which already raise an exception when an object of the wrong class is passed.
32 changes: 30 additions & 2 deletions lib/matplotlib/axis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,8 +640,36 @@ class Ticker(object):
formatter : `matplotlib.ticker.Formatter` subclass
Determines the format of the tick labels.
"""
locator = None
formatter = None
Copy link
Member

Choose a reason for hiding this comment

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

I hope that removing these (public) class attributes isn't going to trip anyone up. Maybe that would involve such an obscure use case that it won't happen.

Copy link
Member

Choose a reason for hiding this comment

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

We did just have that issue, but in this case I suspect it will be ok.

These lines go back to d78e6c4 and I suspect it was done done to make sure that Ticker sub-classes always had the attributes. Putting something into these attributes at the class level does not really make sense.


def __init__(self):
self._locator = None
self._formatter = None

@property
def locator(self):
return self._locator

@locator.setter
def locator(self, locator):
if not isinstance(locator, mticker.Locator):
cbook.warn_deprecated(
"3.2", message="Support for locators that do not subclass "
"matplotlib.ticker.Locator is deprecated since %(since)s and "
"support for them will be removed %(removal)s.")
self._locator = locator

@property
def formatter(self):
return self._formatter

@formatter.setter
def formatter(self, formatter):
if not isinstance(formatter, mticker.Formatter):
cbook.warn_deprecated(
"3.2", message="Support for formatters that do not subclass "
"matplotlib.ticker.Formatter is deprecated since %(since)s "
"and support for them will be removed %(removal)s.")
self._formatter = formatter


class _LazyTickList(object):
Expand Down
0