Skip to content

Simplify OldAutoLocator and AutoDateLocator. #15606

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

Jump to bottom
Merged
merged 1 commit into from
Mar 24, 2020
Merged

Simplify OldAutoLocator and AutoDateLocator. #15606

merged 1 commit into from
Mar 24, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 5, 2019

In both classes the refresh method only sets the _locator attribute,
which is only later called in __call__ -- but __call__ also calls
refresh first. So we can instead inline the contents of refresh
into __call__, get rid of the refresh implementation (inheriting
the do-nothing implementation of the base class) and get rid of the
_locator attribute, as this has no publically observable effect.
Likewise, AutoDateLocator.set_axis can go.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@timhoffm
Copy link
Member

timhoffm commented Nov 5, 2019

refresh() is a public method. People (and user subclasses) may rely on refresh() actually doing something. This is at least an API change.

Semi-OT: May we deprecate OldAutoLocator altogether?

@anntzer
Copy link
Contributor Author

anntzer commented Nov 5, 2019

The point is that it's not doing anything that's publically observable (it's only setting self._locator, but the only place that accesses self._locator is __call__ and __call__ also refreshes the locator first.

Note that the refresh method isn't going away (yet), the parent (do-nothing) implementation is still there.

I actually have a second patch that completely deprecates refresh, but that's a much more complicated task.

Even if we deprecate OldAutoLocator (no strong opinion there), this still applies to AutoDateLocator.

QuLogic
QuLogic reviewed Mar 24, 2020
View reviewed changes
@anntzer
Simplify OldAutoLocator and AutoDateLocator.
1f53c6b 
In both classes the `refresh` method only sets the `_locator` attribute,
which is only later called in `__call__` -- but `__call__` also calls
`refresh` first.  So we can instead inline the contents of `refresh`
into `__call__`, get rid of the `refresh` implementation (inheriting
the do-nothing implementation of the base class) and get rid of the
`_locator` attribute, as this has no publically observable effect.
Likewise, `AutoDateLocator.set_axis` can go.
@QuLogic QuLogic merged commit 38da016 into matplotlib:master Mar 24, 2020
@anntzer anntzer deleted the locator-refresh branch March 24, 2020 19:54
@QuLogic QuLogic added this to the v3.3.0 milestone Mar 24, 2020
@anntzer anntzer mentioned this pull request Mar 30, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
Maintenance
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@anntzer @timhoffm @QuLogic