-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DOC: clean up automated tests section of workflow docs #27696
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
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.
👍 - 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.
@dstansby will try and knock out those changes in the afternoon EST. |
@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. |
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.
I like the tables a lot 👍 - just one suggestion, otherwise looks good to me. Feel free to self merge if you take the suggestion.
04bd09e
to
124edd5
Compare
doc/devel/development_workflow.rst
Outdated
- Type hints | ||
- Errors are displayed as annotations on the pull request diff. | ||
* - CircleCI | ||
- reStructuredText (rst) |
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.
- reStructuredText (rst) | |
- Documentation build |
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.
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)
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.
but actually, let me just link each column to its part of the guide.
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.
Also I can just name that column tests 🤦♀️
doc/devel/development_workflow.rst
Outdated
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: |
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.
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.
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.
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.
doc/devel/development_workflow.rst
Outdated
* - name | ||
- scope | ||
- tips for finding cause of failure |
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.
* - 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.
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.
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.
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.
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.
Made changes, I'm pretty test failures are unrelated |
turn ci information into tables
Test failures are unrelated. May partly be a regression in the recently released version pytest 8.0. |
PR summary
PR checklist