Skip to content
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

Clarify public-ness of some ToolContainerBase APIs. #28169

Merged
merged 1 commit into from May 5, 2024

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 4, 2024

add_toolitem is clearly intended as a helper for add_tool (add_tool sets up a bunch of auxiliary arguments for it which the end-user should not bother with). toggle_toolitem and remove_toolitem also look internal-ish. Document them as such.

Noted while looking at #28166.

PR summary

PR checklist

Comment on lines 3417 to 3421
Note
----
This method is not intended to be called by end-users (who should
instead `.ToolContainerBase.add_tool`); it is provided to be
implemented by third-party backends.
Copy link
Member

@timhoffm timhoffm May 4, 2024

Choose a reason for hiding this comment

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

I would phrase the whole docstring differently:

A hook to add a toolitem to the container.

This hook must be implemented in each backend and contains the backend-specific
code to add a tool; e.g. add an element to the toolbar.

.. warning::
    This is part of the backend implementation and should not be called by end-users.
    They should instead call `.ToolContainerBase.add_tool`.

I intentionally deviated from the "Do something" summary line to set this apart from
regular methods that users will call to perform actions.

Also for the other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

add_toolitem is clearly intended as a helper for add_tool (add_tool sets
up a bunch of auxiliary arguments for it which the end-user should not
bother with).  toggle_toolitem and remove_toolitem also look
internal-ish.  Document them as such.
@timhoffm timhoffm added this to the v3.9.0 milestone May 5, 2024
@timhoffm timhoffm merged commit 9387431 into matplotlib:main May 5, 2024
41 of 42 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request May 5, 2024
tacaswell added a commit that referenced this pull request May 6, 2024
…169-on-v3.9.x

Backport PR #28169 on branch v3.9.x (Clarify public-ness of some ToolContainerBase APIs.)
@anntzer anntzer deleted the tcp branch May 6, 2024 07:01
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.

None yet

2 participants