-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-77024: test.support: Improve documentation #92513
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
This is a rework of python#5774 on current main. I was a bit more conservative in making changes than the original PR. See @csabella's comments on issue python#77024 and the discussion on python#5774 for explanations of several of the changes. Co-authored-by: Cheryl Sabella <[email protected]>
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.
LGTM, thanks.
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.
Thanks, this looks like a big improvement! A few notes:
Set to a filename containing the :data:`FS_NONASCII` character, if it exists. | ||
This guarantees that if the filename exists, it can be encoded and decoded | ||
with the default filesystem encoding. This allows tests that require a | ||
non-ASCII filename to be easily skipped on platforms where they can't work. |
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.
This currently feels like it leads with the implementation, and finishes with why it's useful ("This allows tests that require a non-ASCII filename to be easily skipped on platforms where they can't work"). The other way around might be better.
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 see your point, but I feel like the current sequence is more in line with the surrounding TESTFN_* paragraphs.
Co-authored-by: Alex Waygood <[email protected]>
Thanks for your review @AlexWaygood! |
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.
Just two more nits I spotted, otherwise looks great!
Co-authored-by: Alex Waygood <[email protected]>
Thanks @JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11. |
GH-93291 is a backport of this pull request to the 3.11 branch. |
This is a rework of pythonGH-5774 on current main. I was a bit more conservative in making changes than the original PR. See @csabella's comments on issue pythonGH-77024 and the discussion on pythonGH-5774 for explanations of several of the changes. Co-authored-by: Cheryl Sabella <[email protected]> Co-authored-by: Alex Waygood <[email protected]> (cherry picked from commit 8995177) Co-authored-by: Jelle Zijlstra <[email protected]>
This is a rework of pythonGH-5774 on current main. I was a bit more conservative in making changes than the original PR. See @csabella's comments on issue pythonGH-77024 and the discussion on pythonGH-5774 for explanations of several of the changes. Co-authored-by: Cheryl Sabella <[email protected]> Co-authored-by: Alex Waygood <[email protected]> (cherry picked from commit 8995177) Co-authored-by: Jelle Zijlstra <[email protected]>
This is a rework of GH-5774 on current main. I was a bit more conservative in making changes than the original PR. See @csabella's comments on issue GH-77024 and the discussion on GH-5774 for explanations of several of the changes. Co-authored-by: Cheryl Sabella <[email protected]> Co-authored-by: Alex Waygood <[email protected]> (cherry picked from commit 8995177) Co-authored-by: Jelle Zijlstra <[email protected]>
This is a rework of GH-5774 on current main. I was a bit more conservative in making changes than the original PR. See @csabella's comments on issue GH-77024 and the discussion on GH-5774 for explanations of several of the changes. Co-authored-by: Cheryl Sabella <[email protected]> Co-authored-by: Alex Waygood <[email protected]> (cherry picked from commit 8995177) Co-authored-by: Jelle Zijlstra <[email protected]>
This is a rework of #5774 on current main. I was a bit more
conservative in making changes than the original PR.
See @csabella's comments on issue #77024 and the discussion
on #5774 for explanations of several of the changes.
Co-authored-by: Cheryl Sabella [email protected]