Skip to content

Promote mpltype Sphinx role to a public extension #28289

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 2 commits into from
Jun 26, 2024

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented May 23, 2024

PR summary

When projects derive from our types, but don't override all the docstrings, they may want to use these extensions so that their docs build. Fixes #28234

However, this does mean that when enabled, you get :mpltype: and :rc: roles added globally. I have not fully investigated it, but we may want to instead move to a Sphinx Domain. This would require changing the usage to something like :mpl:type: and :mpl:rc:. However, I do need to investigate how that will interact with intersphinx (as if it doesn't work, then we don't really help downstreams.)

PR checklist

@timhoffm
Copy link
Member

timhoffm commented May 23, 2024

Re: domain. As long as we can declare this non-public (use for inherited docstrings, but not in 3rd party code) we are still free to change the role names in the future, which means, we can wait until the (rather unlikely) case of a conflict with the global names happen and only then switch to domains.

@tacaswell
Copy link
Member

I am wary of saying things are private without a technical (or very obvious naming) way for the user to tell at the point they are using it.

@tacaswell tacaswell added this to the v3.9.1 milestone May 24, 2024
@timhoffm
Copy link
Member

timhoffm commented May 26, 2024

Well, it’s not an average user. Affected are only downstream projects that inherit from our classes and generate documentation. Maybe we could/should have guidelines for them anyway.
Additionally, by default such projects would get an unknown role error. They’ll have to add our new module that defines the roles to their Sphinx conf. And to know that the y have to to this this, they’ll have to read some sort of documentation, in which we can explain the scope. - If you want to be extra careful, you can have our new module with an underscore prefix.

I‘m hesitant to make the roles public unconditionally because they are primarily intended for internal use. Making them public would make it more difficult to make any changes.

When projects derive from our types, but don't override all the
docstrings, they may want to use these extensions so that their docs
build.
@QuLogic QuLogic marked this pull request as ready for review June 1, 2024 01:13
@QuLogic
Copy link
Member Author

QuLogic commented Jun 1, 2024

I added an API change note now, but did not add this anywhere to our docs otherwise.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

For third-party packages that derive types from Matplotlib, our use of custom roles may
prevent Sphinx from building their docs. These custom Sphinx roles are now public solely
Copy link
Member

Choose a reason for hiding this comment

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

We can be more explicit here:

  1. explain the problem: roles in inherited docstings
  2. Explicitly write down the exceptions for all our roles („you may see one on the following exceptions). Since we cannot influence the exception messages and they are not telling by themselves, people searching for the message string is their only way forward. We thus should have the stings found in our docs (and that hopefully later also via google).
  3. State that the addition of the extension should be used to solve the inherited docstring issue, but people should not use these roles explicitly in their docs. (Or rather, we don’t give any guarantees on these. - It might still be convenient to use them, but people should be aware that their docs may break in the future and they might have to adapt, which is not the end of the world for doc builds).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe move the bulk of the explanation to the roles module docstring and give only a very brief summary here, linking to the module docs. That way, it’s easier to update the docs later.

@timhoffm
Copy link
Member

timhoffm commented Jun 1, 2024

@QuLogic I took the liberty to push a commit with the above suggested changes. Feel free to adapt as you see fit.

@ksunden ksunden merged commit 664b457 into matplotlib:main Jun 26, 2024
41 of 42 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jun 26, 2024
@QuLogic QuLogic deleted the public-roles branch June 26, 2024 20:15
QuLogic added a commit that referenced this pull request Jun 26, 2024
…289-on-v3.9.x

Backport PR #28289 on branch v3.9.x (Promote mpltype Sphinx role to a public extension)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants