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.

Sign up for GitHub

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

Jump to bottom

Docs: move sphinx-lint to pre-commit #105750

Merged
merged 2 commits into from
Jun 18, 2023
Merged

Docs: move sphinx-lint to pre-commit #105750

merged 2 commits into from
Jun 18, 2023

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Jun 13, 2023 •
edited by github-actions bot
Loading

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/

AlexWaygood
AlexWaygood approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@AlexWaygood AlexWaygood 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
Member Author

hugovk commented Jun 13, 2023

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)

ezio-melotti
ezio-melotti reviewed Jun 15, 2023
View reviewed changes
Doc/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
Member Author

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.

CAM-Gerlach
CAM-Gerlach approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-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-Gerlach CAM-Gerlach added the needs backport to 3.12 bug and 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.

@hugovk hugovk enabled auto-merge (squash) June 18, 2023 11:33
@hugovk hugovk merged commit bc07c8f into python:main Jun 18, 2023
@hugovk hugovk 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
@hugovk @miss-islington
Docs: move sphinx-lint to pre-commit (pythonGH-105750)
3227dbb 
(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-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 18, 2023
AlexWaygood pushed a commit that referenced this pull request Jun 18, 2023
@miss-islington @hugovk
[3.12] Docs: move sphinx-lint to pre-commit (GH-105750) (#105894)
6baddd9 
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>
This was referenced Aug 21, 2023
Merged
Merged
Merged
@bedevere-bot
Copy link

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

hugovk added a commit that referenced this pull request Aug 22, 2023
@hugovk
[3.11] Docs: move sphinx-lint to pre-commit (GH-105750) (#108276)
9d00379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezio-melotti ezio-melotti ezio-melotti left review comments

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Assignees
No one assigned
Labels
skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hugovk @CAM-Gerlach @miss-islington @bedevere-bot @ezio-melotti @AlexWaygood