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

DOC: Add role for custom informal types like color #27200

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Oct 25, 2023

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 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.

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.
@story645
Copy link
Member

is easily extendable to other types

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.

@timhoffm
Copy link
Member Author

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 type_to_link_target dict in mpltyle_role().

@story645
Copy link
Member

Hmm, wondering if this could/should be like the :envar: directive, where every usage of the role links back to the envar page where that variable is defined:
https://github.com/matplotlib/matplotlib/blob/main/doc/users/installing/environment_variables_faq.rst

where we maintain one page that's the list of types. We've talked about this before as a #9967

@timhoffm
Copy link
Member Author

Possibly, but that's rather orthogonal to the ref implementation. That page would basically define the labels.

@ksunden
Copy link
Member

ksunden commented Oct 26, 2023

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

@story645
Copy link
Member

story645 commented Oct 26, 2023

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:

  • decentralized: each type gets defined in a doc somewhere and those refs get added into the dict
  • centralized: one API page where they all get defined and then we use that role everywhere

and my concern is we'll end up w/ a very confusing mix of both if we don't do any planning.

@jklymak
Copy link
Member

jklymak commented Oct 26, 2023

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.

@story645
Copy link
Member

would be good to have three or four examples of things that should be indexed,

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.

@jklymak
Copy link
Member

jklymak commented Oct 26, 2023

Yes - my suggestion was that it would be good to have at least some of that index in place.

@timhoffm
Copy link
Member Author

timhoffm commented Oct 26, 2023

I don't think this needs to wait for deciding on a definitive index. This already improves on the current situation in two ways:

  • It creates a more consistent type description in the docs. Old: No links, or after docs: adding explanation for color in set_facecolor #27164, the somewhat verbose

    grafik

    new:

    grafik

    This is also more consistent with the standard type formatting, which is just the type name as a link, not any additional "see someplace".

  • This decouples the type link from the format and location of of the index, so stah people can immediately start formalizing all color types without the need to first decide on the index.

If we later decide to change the source formatting of the link to something other than :mpltype:`color` , it will be trival to search/replace.

@story645
Copy link
Member

If we later decide to change the source formatting of the link to something other than :mpltype:color , it will be trival to search/replace.

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.

@timhoffm
Copy link
Member Author

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.

Copy link
Member

@story645 story645 left a 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.

@story645 story645 added the Documentation: build building the docs label Nov 2, 2023
@timhoffm
Copy link
Member Author

timhoffm commented Nov 2, 2023

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.

@timhoffm
Copy link
Member Author

Friendly ping. This needs a second reviewer.

@tacaswell tacaswell merged commit 502b311 into matplotlib:main Dec 20, 2023
43 of 44 checks passed
@timhoffm timhoffm deleted the doc-type-ref branch December 21, 2023 08:50
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.

[Doc]: Document color in a consistent way, including link
5 participants