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: clean up automated tests section of workflow docs #27696

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

story645
Copy link
Member

PR summary

  • Added link to github docs that @ksunden shared and info about what runs workflows are applied to b/c it's useful for debugging actions
  • rearranged things to removed nested bullet lists b/c I find nested bullets hard to read
  • made skipping its own section b/c convenient for linking

PR checklist

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

👍 - I've left some minor suggestsions, but given you've mainly moved text instead of changing it feel free to take them or leave them.

doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
@story645
Copy link
Member Author

story645 commented Jan 25, 2024

@dstansby will try and knock out those changes in the afternoon EST.

@story645
Copy link
Member Author

@dstansby so in making changes, decided that the long bullets made more sense as a table (that's a habit of mine 😅). Stashed that in a second commit - if it looks good to you please squash merge, otherwise I'll drop it and make the original changes.

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I like the tables a lot 👍 - just one suggestion, otherwise looks good to me. Feel free to self merge if you take the suggestion.

doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
@story645 story645 force-pushed the auto-tests branch 14 times, most recently from 04bd09e to 124edd5 Compare January 26, 2024 19:57
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
- Type hints
- Errors are displayed as annotations on the pull request diff.
* - CircleCI
- reStructuredText (rst)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- reStructuredText (rst)
- Documentation build

Copy link
Member Author

@story645 story645 Jan 28, 2024

Choose a reason for hiding this comment

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

Same as mentioned to @dstansby, what the documentation build fails on specifically is usually bad rst (which gonna rework that column so fails on is the title)

Copy link
Member Author

Choose a reason for hiding this comment

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

but actually, let me just link each column to its part of the guide.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I can just name that column tests 🤦‍♀️

Comment on lines 500 to 501
If you know only a subset of CI checks need to be run, you can skip any unneeded CI
checks on individual commits by including the following substrings in commit messages:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you know only a subset of CI checks need to be run, you can skip any unneeded CI
checks on individual commits by including the following substrings in commit messages:
If you know only a subset of CI checks need to be run, you can skip any unneeded CI
checks on individual commits by including the following strings in commit messages:

"sub" does not add anything.

Also, do we want to recommend where to put these strings (first line or separate line at the end)? I'd favor the latter because these are technical control directives and not relevant when skimming through git logs. But then again, there's the exception of AppVeyor 😞. Maybe then let's not get into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't think it's worth documenting cause we can always strip those out before merging and honestly I think basically everything should run through a "don't skip anything" run before merge.

Comment on lines 472 to 474
* - name
- scope
- tips for finding cause of failure
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* - name
- scope
- tips for finding cause of failure
* - Name
- Purpose
- Tips for finding cause of failure

Would handle capitalization like section titles.

Purpose (=What is this for) seems more helpful than scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the reason wrote scope instead of purpose was b/c that column just had the topic (so like type hints is not a purpose, ) but if I rework the column contents than yes purpose makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

And also the problem with purpose is that there's an implied checks in front of every word in the table and that seems unweildy.

doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
@story645
Copy link
Member Author

Made changes, I'm pretty test failures are unrelated

turn ci information into tables
@story645 story645 added this to the v3.9.0 milestone Jan 31, 2024
@timhoffm
Copy link
Member

Test failures are unrelated. May partly be a regression in the recently released version pytest 8.0.

@timhoffm timhoffm merged commit 41d4149 into matplotlib:main Jan 31, 2024
36 of 42 checks passed
@story645 story645 deleted the auto-tests branch January 31, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: devdocs files in doc/devel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants