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

Use :mpltype:color for color types #27557

Merged
merged 2 commits into from
Mar 9, 2024
Merged

Conversation

timhoffm
Copy link
Member

Follow up to #27200.

@timhoffm timhoffm added the Documentation: API files in lib/ and doc/api label Dec 22, 2023
@timhoffm timhoffm added this to the v3.9.0 milestone Dec 22, 2023
@timhoffm timhoffm marked this pull request as draft December 22, 2023 09:23
@timhoffm
Copy link
Member Author

timhoffm commented Dec 22, 2023

I'm unclear why the doc build is failing. I've only added :mpltype:`color` . That this works in principle has been shown in #27200. Also my local doc build works. Likely, I'm overlooking something 😞.

@timhoffm
Copy link
Member Author

Found the bug in the :mpltype: role (first commit). Usage in docstrings: second commit.

@@ -1073,7 +1073,7 @@ def hlines(self, y, xmin, xmax, colors=None, linestyles='solid',
Respective beginning and end of each line. If scalars are
provided, all lines will have the same length.

colors : color or list of colors, default: :rc:`lines.color`
colors : :mpltype:`color` or list of colors, default: :rc:`lines.color`
Copy link
Member

Choose a reason for hiding this comment

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

Why is this one list of colors, while the others were changed to list of color (and some others where changed to list of :mpltype:'color')?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. The canoncial description is "list of [type]", i.e. should be "list of color". However, this is currently quite inconsistent. I've only changed it when changing the text anyway.

  2. For "color or list of color" I've refrained from linking the second color if that would make a a line continuation necessary. But that's debatable of course.

@@ -179,7 +179,7 @@
Minimum length as a multiple of shaft width; if an arrow length
is less than this, plot a dot (hexagon) of this diameter instead.

color : color or color sequence, optional
color : :mpltype:`color` or color sequence, optional
Copy link
Member

Choose a reason for hiding this comment

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

Should color sequence here be list of :mpltype:color?

Copy link
Member Author

@timhoffm timhoffm Feb 23, 2024

Choose a reason for hiding this comment

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

Fixed. - On a general note, I've used a couple of search/replace patterns. It is likely that I have not found all color type definitions with that. I think it's enough to start with the bulk here and fix others later if we find them.

@timhoffm
Copy link
Member Author

Semi-OT: I don't think our GitHub auto-labeling (ref. #27633) makes much sense here. Can we fine-tune this. Or make deactivation possible (maybe through a "no-auto-label" label)? Or should we just live with it?

`inline.interpreted()` already returns a list of nodes. We mustn't wrap
it in another list.
@QuLogic
Copy link
Member

QuLogic commented Mar 9, 2024

Semi-OT: I don't think our GitHub auto-labeling (ref. #27633) makes much sense here. Can we fine-tune this. Or make deactivation possible (maybe through a "no-auto-label" label)? Or should we just live with it?

It only cares about file names, so you can't have it ignore PRs where just docstring are touched, for example.

@QuLogic QuLogic merged commit f007886 into matplotlib:main Mar 9, 2024
42 checks passed
@timhoffm timhoffm deleted the color-type branch March 9, 2024 19:53
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

3 participants