10000 Cleanup AnchoredOffsetbox-related demos. by anntzer · Pull Request #20493 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Cleanup AnchoredOffsetbox-related demos. #20493

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

Merged
merged 1 commit into from
Jun 27, 2021

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Jun 23, 2021

userdemo/anchored_box0{1,2,3} are basically subsets of
misc/anchored_artists. anchored_box04 is slightly more
sophisticated; we may want to also merge it but in a later step.

In anchored_artists, no need to introduce new subclasses (whose
constructor need to support both the kwargs for AnchoredOffsetbox and
the kwargs for the child artist); it is simpler to just create an
AnchoredOffsetbox and set whatever child artist we want on it.

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@anntzer anntzer force-pushed the cleanup-anchored-demos branch from 11dee67 to e650590 Compare June 23, 2021 13:07
@timhoffm
Copy link
Member

In anchored_artists, no need to introduce new subclasses (whose
constructor need to support both the kwargs for AnchoredOffsetbox and
the kwargs for the child artist); it is simpler to just create an
AnchoredOffsetbox and set whatever child artist we want on it.

I have thought through if this is reasonable or not. However, we have these subclasses in matplotlib.offsetbox and just not using them in the examples is only half a solution. There are three prossibilities:

  • Keep them and use them in the examples.
  • Remove them from the examples and deprecate them.
  • Remove them from the examples and discourage their use by adding .. admonition:: Discouraged to the docs; in case a deprecation is not worth it.

@anntzer
Copy link
Contributor Author
anntzer commented Jun 23, 2021

I think we should at least discourage their use: as always, I'd rather encourage people to compose APIs (eg put a TextArea in an AnchoredOffsetbox) rather than introducing new APIs for each combination (eg. AnchoredText).

@anntzer anntzer force-pushed the cleanup-anchored-demos branch 2 times, most recently from f818197 to e20f7cc Compare June 26, 2021 13:06
`userdemo/anchored_box0{1,2,3}` are basically subsets of
`misc/anchored_artists`.  `anchored_box04` is slightly more
sophisticated; we may want to also merge it but in a later step.

In `anchored_artists`, no need to introduce new subclasses (whose
constructor need to support both the kwargs for AnchoredOffsetbox and
the kwargs for the child artist); it is simpler to just create an
AnchoredOffsetbox and set whatever child artist we want on it.
@anntzer anntzer force-pushed the cleanup-anchored-demos branch from e20f7cc to dfd6da1 Compare June 27, 2021 11:52
Copy link
Member
@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I did not follow every change in the annotations tutorial, but it seems to be mostly copy&paste. I trust that you've done this properly.

@timhoffm timhoffm merged commit 6abaf1f into matplotlib:master Jun 27, 2021
@timhoffm timhoffm added this to the v3.5.0 milestone Jun 27, 2021
@anntzer anntzer deleted the cleanup-anchored-demos branch June 27, 2021 16:20
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.

2 participants
0