Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

DOC: initial tags for statistics section of gallery#27569

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 2 commits intomatplotlib:mainfromstory645:tag-stats
Oct 28, 2024

Conversation

@story645
Copy link
Member

@story645story645 commentedDec 26, 2023
edited
Loading

PR summary

Batch of tags for the stats functions, started by@ktnharris22 in#27235

I mostly didn't add to the tags except to flag stuff that's veering into tutorial or might belong in plot-types. I put/propose stashing internal-tags in an ifconfig b/c I think they can be useful on dev as a flag of what to work on and confusing/clutter on any of the stable versions.

PR checklist

@story645story645 added status: upstream fix required Documentation: tagstagging examples + proposing new tags labelsDec 26, 2023
@story645story645 added this to thev3.9.0 milestoneDec 26, 2023
@story645story645force-pushed thetag-stats branch 2 times, most recently frombcc5d03 tob7fcf69CompareDecember 27, 2023 00:23
@QuLogicQuLogic modified the milestones:v3.9.0,v3.9.1May 10, 2024
@QuLogicQuLogic modified the milestones:v3.9.1,v3.9.2,v3.9-docJun 26, 2024
@ksunden
Copy link
Member

Cycling to rebuild docs, now that upstream has been updated

Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

This also needs a rebase.

@story645
Copy link
MemberAuthor

Cleaned this one up, thanks!

@QuLogic
Copy link
Member

The build is broken because of#28820, but fixing that, it'sreally broken becauseinternal: too-much is not a valid tag.

@story645
Copy link
MemberAuthor

internal: too-much is not a valid tag.

how come? (Last I checked, we don't do any validation on tags)

@timhoffm
Copy link
Member

I'd prefer that we don't use the tagging mechanism for internal suff. Making internal labels public is confusing, OTOH packing them behind.. ifconfig is extra effort (and doesn't work if you want to have public tags at the same time).

It would work, ifsphinx-tags had a configuration to exclude certain tags. But that does not exist right now.

@story645
Copy link
MemberAuthor

and doesn't work if you want to have public tags at the same time

That should if you put it behind a new tag directive

@timhoffm
Copy link
Member

Well, I feel uncomfortable, with this duct-tape coding of internal tags via.. ifconfig. It's cluttered and difficult to write for tag authors. But it technically, works and could be refactored later. So I won't block, but please find somebody else to approve.

story645 reacted with thumbs up emoji

@story645
Copy link
MemberAuthor

It's cluttered and difficult to write for tag authors.

I agree, I just don't want to lose this information b/c right now I don't have the bandwidth to figure out how to get it working on the sphinx-tag side.

So I won't block, but please find somebody else to approve.

Thanks for the heads up.

@jklymak
Copy link
Member

Problems with examples should be issues for discussion, not tags.

@story645
Copy link
MemberAuthor

Problems with examples should be issues for discussion, not tags.

Why not both? The idea here is use tags to get a holistic overview and also flag those examples in the dev version. I think that makes it easier to then write an issue, especially if the fix is more holistic.

@jklymak
Copy link
Member

Because tags are for users to find things in the docs. Issues are for maintainers to track issues.

Basically by tagging an example as "too much" you are issuing a judgement. To remove your judgement I would then have to open a PR to change the tag. That is backwards to how an issue works, where there are no committed changes until a PR is approved.

@timhoffm
Copy link
Member

Is it sufficient to create a collection issue for "example rework"? Instead of adding ainternal: needs review tag to the code, add a line in the issue

This is much more lightweight. It can be edited without a PR and review. Also it's possible to add a few words for context to the point. The tag must stand for itself.

The only downside I see is that non-core devs likely don't have the right to edit that issue. But that's not too bad. They can comment on it and we merge the comments in to one top-level list if needed.

jklymak reacted with thumbs up emoji

@story645
Copy link
MemberAuthor

Because tags are for users to find things in the docs.

These are tags that only show up in the dev version, which we generally don't want users using

To remove your judgement I would then have to open a PR to change the tag.

Or request changes on the PR and have a discussion there?

The only downside I see is that non-core devs likely don't have the right to edit that issue.

That's sorta what motivated having the internal tags in the first place - to give contributors and maintainers a quick way to flag examples that need to be looked at, and using the tags machinery to get a quick list with links of those examples.

The issue version ends up being a top down scan all the examples, while the tag version is "oh, looking through this example for other reasons, it also should get added to the list for review".

For me a compromise would maybe be a pinned issue or template for unclear examples w/ tagging - something where the tracking issue isn't getting buried on page 3.

@jklymak
Copy link
Member

I'll stand by my stance documentation tags should not be used to organize issues. There arelots of mechanisms to organize and surface issues on GitHub, specifically tags and projects, and we should use those.

@story645
Copy link
MemberAuthor

removed the internal tags so this can get merged

Comment on lines 222 to 223
# plot-type: speciality, plot-type: scatter, component: ellipse, component: patch,
# domain: statistics,
Copy link
Member

@timhoffmtimhoffmOct 28, 2024
edited
Loading

Choose a reason for hiding this comment

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

We have discussed this somewhere, but can't find the PR right now: I believe we wanted to use a standard formatting for better readability, and this can be

  • one line for short lists, e.g.
    .. tags:: plot-type: speciality, domain: statistics
  • one tag per line for longer lists
    Suggested change
    # plot-type: speciality, plot-type: scatter, component: ellipse, component: patch,
    # domain: statistics,
    # plot-type: speciality
    # plot-type: scatter
    # component: ellipse
    # component: patch
    # domain: statistics

From a quick check, existing tags seem indeed to conform to this formatting.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yeah but this didn't need to be line wrapped/trigger the PEP 8 test and I thought that was the benchmark for "long"

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yeah but this didn't need to be line wrapped/trigger the PEP 8 test and I thought that was the benchmark for "long"

Copy link
Member

Choose a reason for hiding this comment

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

My definition of "long" is "does not fit on a single line". What's your cut-off? 😄

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The linter yelling

Copy link
Member

Choose a reason for hiding this comment

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

But you started a second line with# domain: statistics,. To extrapolate, I could add more tags there, and when that's full, start a third line and continue.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

🤦‍♀️ sorry for being stupidly stubborn, I thought they were all on one line and didn't catch the #

Copy link
Member

Choose a reason for hiding this comment

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

No problem 😁

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

ok, just pushed up all the changes.

Co-authored-by: katie <katie@unrefugees.org>Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@timhoffmtimhoffm merged commitd7ce220 intomatplotlib:mainOct 28, 2024
22 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestOct 28, 2024
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestOct 28, 2024
@story645story645 deleted the tag-stats branchOctober 28, 2024 23:42
timhoffm added a commit that referenced this pull requestOct 29, 2024
…569-on-v3.9.xBackport PR#27569 on branch v3.9.x (DOC: initial tags for statistics section of gallery)
greglucas added a commit that referenced this pull requestOct 29, 2024
…569-on-v3.9.2-docBackport PR#27569 on branch v3.9.2-doc (DOC: initial tags for statistics section of gallery)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@timhoffmtimhoffmtimhoffm approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees

No one assigned

Labels

Documentation: examplesfiles in galleries/examplesDocumentation: tagstagging examples + proposing new tags

Projects

None yet

Milestone

v3.9-doc

Development

Successfully merging this pull request may close these issues.

5 participants

@story645@ksunden@QuLogic@timhoffm@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp