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

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 04:41
@ezio-melotti
Copy link
Member Author

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

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:

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

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 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 ezio-melotti mentioned this pull request May 6, 2022
5 tasks
@ezio-melotti
Copy link
Member Author

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

@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 18:22
@ezio-melotti ezio-melotti requested a review from abalkin as a code owner May 14, 2022 18:40
@ezio-melotti
Copy link
Member Author

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

Looks like it works on WIndows too.

@ezio-melotti ezio-melotti merged commit 953ab07 into python:main May 15, 2022
@ezio-melotti ezio-melotti deleted the make-check-default-role branch May 15, 2022 15:34
@ezio-melotti ezio-melotti added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels May 15, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

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

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 pull request 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 bedevere-bot removed the needs backport to 3.11 only security fixes label May 15, 2022
@bedevere-bot
Copy link

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

@ezio-melotti ezio-melotti removed needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels May 15, 2022
ezio-melotti added a commit that referenced this pull request 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

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.

8 participants