Skip to content

Conversation

nineteendo
Copy link
Contributor

@nineteendo nineteendo commented Apr 10, 2024

@nineteendo
Copy link
Contributor Author

@ezio-melotti
Copy link
Member

Thanks for the PR!
I think it would be more appropriate to mention pre-commit in the Setup and Building page, since installing pre-commit is something that only needs to be done once, not for every pull request.

The best place would probably be as a subsection of "Installing Git". The step-by-step guide you edited already links there where it lists the prerequisites.

So I would suggest to:

  1. Add an "Installing pre-commit" subsection under "Installing Git", explaining how to install/set up pre-commit.
  2. Leave the note in the step-by-step guide about fixing errors reported by pre-commit. The note could also link to the "Installing pre-commit" subsection.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

I added some comments after a quick read-through.

nineteendo and others added 2 commits April 11, 2024 09:10
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@nineteendo
Copy link
Contributor Author

nineteendo commented Apr 11, 2024

Could you check what's going wrong with the build? It doesn't tell me anything useful. Hmm, duplicate target name.

@erlend-aasland erlend-aasland changed the title Add pre-commit setup to the Lifecycle of a pull request #1305 Add pre-commit setup to the Lifecycle of a pull request Apr 11, 2024
@erlend-aasland erlend-aasland linked an issue Apr 11, 2024 that may be closed by this pull request
@nineteendo
Copy link
Contributor Author

Do you think it's now clear enough?

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
nineteendo and others added 6 commits April 11, 2024 10:10
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Looks good; thanks @nineteendo, @hugovk and @ezio-melotti.

@nineteendo
Copy link
Contributor Author

I moved pre-commit under getting the source code: https://cpython-devguide--1306.org.readthedocs.build/getting-started/setup-building/#install-pre-commit-as-a-git-hook. You need to manually install the hook for every repository.

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

@nineteendo
Copy link
Contributor Author

I think this can be merged now. Am I right?

@erlend-aasland erlend-aasland merged commit 8c95d16 into python:main Apr 11, 2024
@erlend-aasland
Copy link
Contributor

Thank you!

@encukou
Copy link
Member

encukou commented Jul 27, 2024

This will install software on my machine.
Who vouches for the security of that software?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pre-commit setup to the Lifecycle of a pull request
5 participants