GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Docs: move sphinx-lint to pre-commit by hugovk · Pull Request #105750 · python/cpython

Conversation

hugovk commented Jun 13, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Docs: move sphinx-lint to pre-commit

3b96878

hugovk requested review from AlexWaygood and ezio-melotti as code owners

June 13, 2023 20:56

bedevere-bot added the awaiting core review label

Jun 13, 2023

hugovk added skip issue skip news labels

Jun 13, 2023

AlexWaygood approved these changes

Jun 13, 2023

View reviewed changes

AlexWaygood left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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)!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Jun 13, 2023

hugovk commented Jun 13, 2023

Copy link

Copy Markdown

Member Author

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)

AlexWaygood reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reviewed

Jun 15, 2023

View reviewed changes

Comment thread

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

ezio-melotti Jun 15, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hugovk Jun 15, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reacted with thumbs up emoji

All reactions

👍 1 reaction

CAM-Gerlach approved these changes

Jun 16, 2023

View reviewed changes

CAM-Gerlach left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, thanks @hugovk

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

CAM-Gerlach added the needs backport to 3.12 only security fixes label

Jun 16, 2023

CAM-Gerlach commented Jun 16, 2023

Copy link

Copy Markdown

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.

ezio-melotti reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge remote-tracking branch 'upstream/main' into docs-mv-sphinx-lint…

be2a3c7

…-to-pre-commit

hugovk enabled auto-merge (squash)

June 18, 2023 11:33

hugovk merged commit bc07c8f into python:main

Jun 18, 2023

hugovk deleted the docs-mv-sphinx-lint-to-pre-commit branch

June 18, 2023 11:52

miss-islington commented Jun 18, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Jun 18, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jun 18, 2023


          Docs: move sphinx-lint to pre-commit (pythonGH-105750)

3227dbb

(cherry picked from commit bc07c8f)

Co-authored-by: Hugo van Kemenade <[email protected]>

bedevere-bot commented Jun 18, 2023

Copy link

Copy Markdown

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

hugovk reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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


          [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 <[email protected]>

This was referenced Aug 21, 2023

Run sphinx-lint on Misc/NEWS.d/next/ #108212

Merged

[3.11] Trim trailing whitespace and test on CI (GH-104275) #108215

Merged

[3.11] Docs: move sphinx-lint to pre-commit (GH-105750) #108276

Merged

bedevere-bot commented Aug 22, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk added a commit that referenced this pull request

Aug 22, 2023


          [3.11] Docs: move sphinx-lint to pre-commit (GH-105750) (#108276)

9d00379

Reviewers

ezio-melotti

ezio-melotti left review comments

CAM-Gerlach

CAM-Gerlach approved these changes

AlexWaygood

AlexWaygood approved these changes

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

hugovk commented Jun 13, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

hugovk commented Jun 13, 2023

Uh oh!

ezio-melotti Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

hugovk Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach left a comment

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Jun 16, 2023

Uh oh!

miss-islington commented Jun 18, 2023

Uh oh!

bedevere-bot commented Jun 18, 2023

Uh oh!

bedevere-bot commented Aug 22, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hugovk commented Jun 13, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.