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

Remove almost all unpaired backticks in docstrings #119231

Merged
merged 1 commit into from
May 22, 2024

Conversation

geofft
Copy link
Contributor

@geofft geofft commented May 20, 2024

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

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.


📚 Documentation preview 📚: https://cpython-previews--119231.org.readthedocs.build/

@geofft geofft force-pushed the fix-docstring-backtick branch 2 times, most recently from 678a269 to ceeabbe Compare May 20, 2024 18:50
@nineteendo
Copy link
Contributor

nineteendo commented May 20, 2024

I think you need to adjust this (or restore the error message):

def test_bad_new_super(self):
with self.assertRaisesRegex(
TypeError,
'do not use .super...__new__;',
):

@carljm
Copy link
Member

carljm commented May 20, 2024

@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.

@sethmlarson sethmlarson removed their request for review May 20, 2024 21:56
@geofft
Copy link
Contributor Author

geofft commented May 20, 2024

I'd personally feel more comfortable limiting this change to docstrings.

OK, let me split that out, thanks for the review!

Also, rebasing on latest main might fix the docs failure in CI.

Yes, it will. I raced with a change to the CI rules.

@geofft geofft force-pushed the fix-docstring-backtick branch from de783c3 to a1bb778 Compare May 20, 2024 22:40
@geofft
Copy link
Contributor Author

geofft commented May 20, 2024

OK, I think this is now just docstrings + a couple of comments in files where I was already making docstring changes.

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.

@nineteendo
Copy link
Contributor

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"

Copy link
Contributor

@willingc willingc left a 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.

Modules/pyexpat.c Show resolved Hide resolved
Modules/clinic/pyexpat.c.h Show resolved Hide resolved
@carljm
Copy link
Member

carljm commented May 21, 2024

This looks good to me. There are changes to Misc/sbom.spdx.json but the CI check doesn't seem happy with those changes. Does it change anything if you run make regen-sbom again?

@geofft geofft force-pushed the fix-docstring-backtick branch from a1bb778 to bd11167 Compare May 21, 2024 18:34
@geofft geofft changed the title Remove almost all unpaired backticks Remove almost all unpaired backticks in docstrings May 21, 2024
@nineteendo
Copy link
Contributor

From the https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

@geofft
Copy link
Contributor Author

geofft commented May 21, 2024

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.

@nineteendo
Copy link
Contributor

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.
@geofft geofft force-pushed the fix-docstring-backtick branch from bd11167 to 4a80aea Compare May 22, 2024 15:21
@nineteendo
Copy link
Contributor

nineteendo commented May 22, 2024

@geofft, please do not force push, use git push or git merge <branch>.

Show schema

merge-squash-rebase

Note: if you use squash, it doesn't commit.

@AlexWaygood
Copy link
Member

@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 :-)

@nineteendo
Copy link
Contributor

OK, (hmm that's all caps too). I simply wanted to highlight "not".

@geofft
Copy link
Contributor Author

geofft commented May 22, 2024

@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?

@geofft
Copy link
Contributor Author

geofft commented May 22, 2024

In any case - I think this is ready to merge now, can we merge it before there are further merge conflicts? :)

@nineteendo
Copy link
Contributor

In this case, adding a merge commit would have made it harder to figure out what I did, I think?

Actually not, because GitHub shows a condensed view in that case instead of all unrelated changes.
I was already wondering if you made other changes.

In any case, if you force push, always explain why you did it and why you didn't make a merge commit.

@carljm carljm merged commit ef17252 into python:main May 22, 2024
42 checks passed
@carljm
Copy link
Member

carljm commented May 22, 2024

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.)

@geofft geofft deleted the fix-docstring-backtick branch May 25, 2024 02:17
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants