-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DOC: Add role for custom informal types like color #27200
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
Conversation
The role is intended to be used for type references in docstrings like ``` Parameters ---------- color : :mpltype:`color` ``` It is easily extendable to other types. The naming `mpltype` was a quick choice. I'm open to better names. This PR contains one example usage in `widgets.Button` so that one can see the effect in the built docs. Systematic application throughout the codebase should be deferred to a separate PR. Closes matplotlib#24859. Formalizes matplotlib#27164.
cfd284f
to
689ab84
Compare
Hatch is the other one I'm thinking of immediately, but also wondering if there's a way to autogenerate the ones that are a literal - either from typing stubs (where we define the literal list as a subtype) or the validators or something. |
I wouldn't go for autogeneration. You would have to specify a translation from the type to the link target (e.g. type -> type_def, which would currently fail because we map color -> colors_def). Also, that's too much magic and implicitness. There's basically no effort in manually maintaining the |
Hmm, wondering if this could/should be like the where we maintain one page that's the list of types. We've talked about this before as a #9967 |
Possibly, but that's rather orthogonal to the ref implementation. That page would basically define the labels. |
I will state that we now do have ColorType somewhat defined (we could add a docstring to it which links to the color reference itself, though it doesn't have one yet) https://matplotlib.org/stable/api/typing_api.html#matplotlib.typing.ColorType |
Sorry Kyle I edited your comment! Tried to put it back, really wish there was a way to lock comments :/ That's probably the page that would make sense as the basis for what I'm after (+- that I wish stuff like colortype wasn't replaced by a tuple) but I also won't hold up this PR for something like that. I'm just thinking through/concerned about us ending up w/ two approaches for how we define our types in docs:
and my concern is we'll end up w/ a very confusing mix of both if we don't do any planning. |
If we are going to have a custom role that looks up an index like this, it would be good to have three or four examples of things that should be indexed, otherwise, it seems just as easy to reference the appropriate page. |
color, hatch, markerstyles, line styles, capstyles,, join styles, really probably any of the aesthetic properties that aren't a float and even then, this would be a good way to document something like the line width units. |
Yes - my suggestion was that it would be good to have at least some of that index in place. |
I don't think this needs to wait for deciding on a definitive index. This already improves on the current situation in two ways:
If we later decide to change the source formatting of the link to something other than |
The index approach proposes using the same directive, just defining it initially on one page. I'm not opposed to a staged start here then move - I just don't want a half/half. |
I mostly care about having reasonably formatted link that goes somewhere informative, because that affects usability. You are free to order and design the targets of the links. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely going to be an improvement over status quo and switching over to a definitional role down the line shouldn't be hard. My only :/ is having the list definition in the function like that cause I can see that becoming a pain as it grows.
I don't think we need to be too worried about this. I expect this to grow to not more than a handful of types, so needing to change the list should be rare. |
Friendly ping. This needs a second reviewer. |
The role is intended to be used for type references in docstrings like
It is easily extendable to other types.
The naming
:mpltype:
was a quick choice. I'm open to other names. (Note that:type:
seem to be available, but I'm inclinded to say it's too generic: People will find it in the code and have a hard time figuring out where it comes from.)This PR contains one example usage in
widgets.Button
so that one can see the effect in the built docs. Systematic application throughout the codebase should be deferred to a separate PR.Closes #24859.
Formalizes #27164.