-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
Remove almost all unpaired backticks in docstrings #119231
Conversation
678a269
to
ceeabbe
Compare
I think you need to adjust this (or restore the error message): Lines 447 to 451 in c0d81b2
|
ceeabbe
to
de783c3
Compare
@geofft To me this PR now looks much bigger than necessary. This quoting style may be uncommon, but in most contexts there's nothing particularly wrong with it. I think the Sphinx failures are sufficient reason to avoid it in docstrings that may end up being parsed by Sphinx, but I don't see much reason to cause churn in other files (e.g. the docs Makefile, or various code comments, or especially in error messages which could in principle affect user code.) So I'd personally feel more comfortable limiting this change to docstrings. Also, rebasing on latest main might fix the docs failure in CI. |
OK, let me split that out, thanks for the review!
Yes, it will. I raced with a change to the CI rules. |
de783c3
to
a1bb778
Compare
OK, I think this is now just docstrings + a couple of comments in files where I was already making docstring 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.
There appear to be no remaining changes on files I maintain. I haven't checked the rest but the changes I did check look good.
Could you update the generated files? diff --git a/Misc/sbom.spdx.json b/Misc/sbom.spdx.json
index 52fc058..b60adcf 100644
--- a/Misc/sbom.spdx.json
+++ b/Misc/sbom.spdx.json
@@ -188,11 +188,11 @@
"checksums": [
{
"algorithm": "SHA1",
- "checksumValue": "be21968aa6e358cb2f193f96d05df806fdf1575c"
+ "checksumValue": "fed1311be8577491b7f63085a27014eabf2caec8"
},
{
"algorithm": "SHA256",
- "checksumValue": "debff6d64b3ef4915873857c315a43193e3fa3e51be11ea19bcbc9d0451985dd"
+ "checksumValue": "3dc233eca5fa1bb7387c503f8a12d840707e4374b229e05d5657db9645725040"
}
],
"fileName": "Modules/expat/xmlparse.c" |
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.
Overall, this looks fine to me. I would like someone to take a look at the changes in the .c
and .h
files. Thanks.
This looks good to me. There are changes to |
a1bb778
to
bd11167
Compare
From the https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide
|
Oh, I didn't realize CPython does squash-and-merge. Thanks for the pointer. In this case you can use the "compare" link on the right to compare the diff - the change from the previous is just Misc/sbom.spdx.json. |
That wouldn't work with multiple commits, it's easier to look through the history from the commits tab and you get a useful overview off all changes since you last looked in notifications. |
As reported in python#117847 and python#115366, an unpaired backtick in a docstring tends to confuse e.g. Sphinx running on subclasses of standard library objects, and the typographic style of using a backtick as an opening quote is no longer in favor. Convert almost all uses of the form The variable `foo' should do xyz to The variable 'foo' should do xyz and also fix up miscellaneous other unpaired backticks (extraneous / missing characters). No functional change is intended here other than in human-readable docstrings.
bd11167
to
4a80aea
Compare
@geofft, please do not force push, use |
@nineteendo, thanks for helping review PRs by other contributors and letting them know about our usual policies and workflow. However, please try to avoid using ALL CAPS when addressing other people in the CPython repo. While I'm sure you didn't mean it in this way, if someone uses ALL CAPS online, it can give the impression that they're shouting or angry :-) |
OK, (hmm that's all caps too). I simply wanted to highlight "not". |
@nineteendo, I was rebasing to resolve a merge conflict. Is there really a preference to resolve merge conflicts by adding merge commits? Another reviewer above specifically requested rebasing (not merging). My reading of the document you linked was that avoiding force-pushing was a preference, not a requirement. In this case, adding a merge commit would have made it harder to figure out what I did, I think? |
In any case - I think this is ready to merge now, can we merge it before there are further merge conflicts? :) |
Actually not, because GitHub shows a condensed view in that case instead of all unrelated changes. In any case, if you force push, always explain why you did it and why you didn't make a merge commit. |
Merged, thanks @geofft for the contribution! (My fault on confusing the issue re merge vs rebase. It is true that typically even for resolving merge conflicts we do merge rather than rebase and force-push, but sometimes we do still casually use the word "rebase" for this 🤦 But none of this is a hard-and-fast rule, there was no problem with your force-push on this PR. The main reason to prefer merge is to avoid accidentally invalidating someone's in-progress review comments.) |
As reported in python#117847 and python#115366, an unpaired backtick in a docstring tends to confuse e.g. Sphinx running on subclasses of standard library objects, and the typographic style of using a backtick as an opening quote is no longer in favor. Convert almost all uses of the form The variable `foo' should do xyz to The variable 'foo' should do xyz and also fix up miscellaneous other unpaired backticks (extraneous / missing characters). No functional change is intended here other than in human-readable docstrings.
As reported in #117847 and #115366, an unpaired backtick in a docstring tends to confuse e.g. Sphinx running on subclasses of standard library objects, and the typographic style of using a backtick as an opening quote is no longer in favor. Convert almost all uses of the form
to
and also fix up miscellaneous other unpaired backticks (extraneous / missing characters).
No functional change is intended here other than in human-readable docstrings.
📚 Documentation preview 📚: https://cpython-previews--119231.org.readthedocs.build/