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

Restore default role check in make check. #92290

Merged
merged 11 commits into from May 15, 2022

Conversation

ezio-melotti
Copy link
Member

@ezio-melotti ezio-melotti commented May 4, 2022

This PR restores the check for the default role (see GH-92289) by doing two things. First, it sets the severity to 0 adding --severity=0, so that failing the default role check results in an error. This however also enables the long lines check, so it explicitly disabled it with --disable='line too long'. There are currently no other checks with severity=0.

This PR only fixes Makefile. If this approach is sound, make.bat should be fixed too.

Using --enable instead might be a better approach, once sphinx-contrib/sphinx-lint#27 lands.

As a side note, it took me a bit to figure out that --disable= expected a message rather than the name of a checker.

cc @JulienPalard

@ezio-melotti ezio-melotti self-assigned this May 4, 2022
@ezio-melotti ezio-melotti marked this pull request as draft May 4, 2022
@ezio-melotti
Copy link
Member Author

@ezio-melotti ezio-melotti commented May 4, 2022

With this PR, the check successfully failed:

PATH=./venv/bin:$PATH sphinx-lint -i tools -i ./venv -i README.rst --severity=0 --disable='line too long'
No problems found.
PATH=./venv/bin:$PATH sphinx-lint ../Misc/NEWS.d/next/ --severity=0 --disable='line too long'
[0] ../Misc/NEWS.d/next/Library/2022-04-26-18-02-44.gh-issue-91928.V0YveU.rst:1: default role used (hint: for inline code, use double backticks)
[0] ../Misc/NEWS.d/next/Documentation/2022-04-24-22-09-31.gh-issue-91888.kTjJLx.rst:1: default role used (hint: for inline code, use double backticks)
2 problems with severity 0 found.
make: *** [Makefile:217: check] Error 1
##[error]Bash exited with code '2'.

Doc/Makefile Show resolved Hide resolved
@ezio-melotti
Copy link
Member Author

@ezio-melotti ezio-melotti commented May 4, 2022

I added the same changes to make.bat, but I don't have a Windows machine to test them (cc @python/windows-team).

make.bat seems outdated compared to Makefile, so I made a couple of changes to make them converge:

  • b973542 added the sphinx-lint check to Misc/NEWS.d/next (see #86404 (comment))
  • b3f1f59 added README.rst to the ignored files, but I don't see a reason to ignore it, so I removed it from Makefile to align it with make.bat instead (see
    #104 (comment))

In addition, bfba2cd added the venv dir to the ignored dirs, but make.bat doesn't seem to use venv, so no change was required.

@vstinner
Copy link
Member

@vstinner vstinner commented May 5, 2022

The CI fails:


Error: [0] ../Misc/NEWS.d/next/Documentation/2022-04-24-22-09-31.gh-issue-91888.kTjJLx.rst:1: default role used (hint: for inline code, use double backticks)
Error: [0] ../Misc/NEWS.d/next/Library/2022-04-26-18-02-44.gh-issue-91928.V0YveU.rst:1: default role used (hint: for inline code, use double backticks)

@ezio-melotti
Copy link
Member Author

@ezio-melotti ezio-melotti commented May 5, 2022

Yes, I'm leaving the failures so that people can test if they are detected on Windows too. If they are, I'll fix them and merge the PR.

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented May 5, 2022

Tried the patch out on my Windows machine:

C:\Users\alexw\coding\cpython>"Doc/make.bat" check
Using py -3.10 (found 3.10 with py.exe)
Error: path too does not exist
Error: path too does not exist

(I'll be honest, I usually don't bother with the .bat file when I'm writing a docs patch. I usually use sphinx-build to build the docs locally, git diff to check for trailing whitespace, and sphinx-lint in CPython's CI to check for markup errors.)

Doc/make.bat Outdated Show resolved Hide resolved
@ezio-melotti
Copy link
Member Author

@ezio-melotti ezio-melotti commented May 7, 2022

Thanks everyone for the feedback and testing! I'll wait for the feedback of @JulienPalard before fixing the 2 failures and merging the PR.

@JulienPalard
Copy link
Member

@JulienPalard JulienPalard commented May 7, 2022

@ezio-melotti Don't you prefer waiting for sphinx-contrib/sphinx-lint#27 to be merged, avoiding two commits on cpython doing the same thing?

It would look like sphinx-lint --enable default-role, which is more transparent than --severity 0 --disable 'line too long'.

Doc/Makefile Outdated Show resolved Hide resolved
Co-authored-by: Julien Palard <julien@palard.fr>
@ezio-melotti ezio-melotti marked this pull request as ready for review May 14, 2022
@ezio-melotti ezio-melotti requested a review from abalkin as a code owner May 14, 2022
@ezio-melotti
Copy link
Member Author

@ezio-melotti ezio-melotti commented May 14, 2022

The PR should be ready to be merged.
@AlexWaygood / @zware: can you check if it works on Windows? (you might have to update sphinx-lint)

@ezio-melotti
Copy link
Member Author

@ezio-melotti ezio-melotti commented May 14, 2022

Looks like it works on WIndows too.

@ezio-melotti ezio-melotti merged commit 953ab07 into python:main May 15, 2022
13 checks passed
@ezio-melotti ezio-melotti deleted the make-check-default-role branch May 15, 2022
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 15, 2022

Thanks @ezio-melotti for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 15, 2022

Thanks @ezio-melotti for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 15, 2022

Thanks @ezio-melotti for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 15, 2022

Sorry @ezio-melotti, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 953ab0795243900ccccaaca069d932730a86fc20 3.9

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 15, 2022

Sorry, @ezio-melotti, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 953ab0795243900ccccaaca069d932730a86fc20 3.10

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 15, 2022

Sorry @ezio-melotti, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 953ab0795243900ccccaaca069d932730a86fc20 3.11

ezio-melotti added a commit to ezio-melotti/cpython that referenced this issue May 15, 2022
* Restore default role check in `make check`.

* Options first, then files.

* Update `make.bat` too.

* Add a comment explaining the extra options.

* No reason to ignore the README.rst.

* Enable default-role check in sphinx-lint.

Co-authored-by: Julien Palard <julien@palard.fr>

* Update sphinx-lint default-role check.

* Fix use of the default role in the docs.

* Update make.bat to check for the default role too.

* Fix comment in make.bat.

Co-authored-by: Julien Palard <julien@palard.fr>
(cherry picked from commit 953ab07)

Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 15, 2022

GH-92821 is a backport of this pull request to the 3.11 branch.

ezio-melotti added a commit that referenced this issue May 15, 2022
* Restore default role check in `make check`.

* Options first, then files.

* Update `make.bat` too.

* Add a comment explaining the extra options.

* No reason to ignore the README.rst.

* Enable default-role check in sphinx-lint.

Co-authored-by: Julien Palard <julien@palard.fr>

* Update sphinx-lint default-role check.

* Fix use of the default role in the docs.

* Update make.bat to check for the default role too.

* Fix comment in make.bat.

Co-authored-by: Julien Palard <julien@palard.fr>
(cherry picked from commit 953ab07)

Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
@JulienPalard
Copy link
Member

@JulienPalard JulienPalard commented May 15, 2022

Thanks @ezio-melotti!

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.

None yet

8 participants