Skip to content

Conversation

timhoffm
Copy link
Member

The tick class hierarchy has a standard mechanism for creating ticks
via _get_tick() The only element that is changing in these functions
is the tick class. All other logic is identical.

This PR makes the tick class a class attribute of Axis so that we can
use one generic _get_tick implementation across all Axis subclasses.

Note that users/downstream libraries can still define their own
_get_tick() methods on subclasses if they want to have a different
behavior.

@timhoffm timhoffm added this to the v3.6.0 milestone Apr 14, 2022
Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Good idea! Maybe worth an API change note?

@timhoffm
Copy link
Member Author

Good idea! Maybe worth an API change note?

Unsure. Who do we want to reach with the note? This is not affecting users or the working of downstream packages. The only people who could be interested are implementers of Axises. They might be able to remove their _get_tick() method, but that would still break their compatibility with all previous versions of matplotlib. So likely, they still won’t switch right now.

OFFSETTEXTPAD = 3
# The class used to create tick instances.
# Must be overwritten in subclasses.
_TICK_CLASS = None
Copy link
Contributor

@anntzer anntzer Apr 15, 2022

Choose a reason for hiding this comment

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

I don't think this needs to be uppercased.

You could also write

@_api.classproperty
def _tick_class(cls):
    raise NotImplementedError(...)

and in the subclasses just directly assign _tick_class = XTick.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lowercased.

IMHO It's a bit simpler if we define the attribute in the base class. A not-implemented property as a placeholder that is overwritten has a slightly nicer error handling but is more difficult to understand. Since there is only one usage where we need the error handling, let's keep it simple.

@anntzer
Copy link
Contributor

anntzer commented Apr 15, 2022

Agreed this doesn't really need an API change note.

@timhoffm timhoffm force-pushed the get_tick-simplification branch from 98fadf5 to c389023 Compare April 15, 2022 09:38
The tick class hierarchy has a standard mechanism for creating ticks
via `_get_tick()` The only element that is changing in these functions
is the tick class. All other logic is identical.

This PR makes the tick class a class attribute of Axis so that we can
use one generic `_get_tick` implementation across all Axis subclasses.

Note that users/downstream libraries can still define their own
`_get_tick()` methods on subclasses if they want to have a different
behavior.
@timhoffm timhoffm force-pushed the get_tick-simplification branch from c389023 to 120328f Compare April 15, 2022 09:41
@anntzer anntzer merged commit 9cf03c0 into matplotlib:main Apr 15, 2022
@timhoffm timhoffm deleted the get_tick-simplification branch April 15, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants