Skip to content

Conversation

@hugovk
Copy link
Member

@hugovkhugovk commented Jun 13, 2023

Move sphinx-lint to pre-commit, and so out of the Docs' various requirements.txt files.

Change make -C Doc check to run pre-commit instead of only sphinx-lint.

pre-commit is run on the CI in lint.yml, so we no longer need to run make check there.


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

Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a great reviewer for the change to the Makefile, but everything else LGTM (and I approve of the idea)!

@hugovk
Copy link
MemberAuthor

Thanks!

I should also mention I based the Makefile stuff from the peps repo:

https://github.com/python/peps/blob/1b0666eafa3d6ca2c5804f7ed41e49ebb74ca831/Makefile#L62-L66

(Which may have come from similar things in https://github.com/python-pillow/Pillow/blob/main/Makefile)

$(SPHINXLINT) -i tools -i $(VENVDIR) --enable default-role
$(SPHINXLINT) --enable default-role ../Misc/NEWS.d/next/
check: venv
$(VENVDIR)/bin/python3 -m pre_commit --version > /dev/null ||$(VENVDIR)/bin/python3 -m pip install pre-commit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why this is installed like this instead of adding it to requirements.txt?
Everything else LGTM.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I think to prefer running pre-commit when committing to Git, and avoid installing it unless we really need to - usually we don't need it installed with all the other venv things.

Copy link
Member

@CAM-GerlachCAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @hugovk

@CAM-GerlachCAM-Gerlach added the needs backport to 3.12 only security fixes label Jun 16, 2023
@CAM-Gerlach
Copy link
Member

Since we also moved the 3.12 branch to using pre-commit and this is just a background infra change, I marked this for backport there too.

@hugovkhugovk enabled auto-merge (squash) June 18, 2023 11:33
@hugovkhugovk merged commit bc07c8f into python:mainJun 18, 2023
@hugovkhugovk deleted the docs-mv-sphinx-lint-to-pre-commit branch June 18, 2023 11:52
@miss-islington
Copy link
Contributor

Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 18, 2023
(cherry picked from commit bc07c8f) Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@bedevere-bot
Copy link

GH-105894 is a backport of this pull request to the 3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12 only security fixes label Jun 18, 2023
AlexWaygood pushed a commit that referenced this pull request Jun 18, 2023
Docs: move sphinx-lint to pre-commit (GH-105750) (cherry picked from commit bc07c8f) Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@bedevere-bot
Copy link

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

Sign up for freeto 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

@hugovk@CAM-Gerlach@miss-islington@bedevere-bot@ezio-melotti@AlexWaygood