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

docs: add suggested pre-commit config #16771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jriddy
Copy link
Contributor

@jriddy jriddy commented Sep 4, 2022

Adds a section describing a possible working pre-commit config, using pass_filenames and --unmatched-cli-globs=ignore

[ci skip-rust]

[ci skip-build-wheels]
@jriddy jriddy force-pushed the docs/tailor-pre-commit-suggestion branch from b6a5faa to d72ece1 Compare September 4, 2022 11:45
@sureshjoshi
Copy link
Member

Question on this: Is this for use cases where pre-commit is already in use for the repo?
Secondary question: Do we have anywhere in the docs (outside of Pants contribution info) specifying how to create a basic git hook? Ala https://sureshjoshi.com/development/pants-plugin-code-quality#pre-commit-hooks

@jriddy
Copy link
Contributor Author

jriddy commented Sep 5, 2022

Question on this: Is this for use cases where pre-commit is already in use for the repo?

Yeah that was the intention

Secondary question: Do we have anywhere in the docs (outside of Pants contribution info) specifying how to create a basic git hook? Ala https://sureshjoshi.com/development/pants-plugin-code-quality#pre-commit-hooks

No, perhaps we should add this. That would be a more natural place for this than CI docs. We could mention hand-rolling hooks like you described or using tools like pre-commit dot com as well.

Mostly I want to highlight the gotchyas around using the --changed-*family of options when dealing with unstaged files and potentially re-named refs in local repos, as well as Pants' behaviors when handling globs it doesn't recognize. But a whole pre-commit, or better yet, "How to help developers test locally" section might be warranted.

@jriddy
Copy link
Contributor Author

jriddy commented Sep 5, 2022

Btw, how do I add a category label? Is that a right given only to maintainers?

@stuhood
Copy link
Member

stuhood commented Sep 6, 2022

Btw, how do I add a category label? Is that a right given only to maintainers?

Contributors can as well: I'll ping you about that offline.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks!


To allow developers to check code before committing or pushing, you can supply a [pre-commit](https://pre-commit.com/) hook that will run Pants commands before. This can help have a faster feedback loop and reduce load and cost on your CI systems.

However, using `--changed-since=origin/main` is not as useful locally. Developers my have unstaged or untracked files lying around in their working directory, or their Git references may be out of date or named differently than the ones you use in CI. Instead, it's better to leverage pre-commit's ability to pass changed filenames to commands to Pants, and tell Pants to ignore files it's been told not to track:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
However, using `--changed-since=origin/main` is not as useful locally. Developers my have unstaged or untracked files lying around in their working directory, or their Git references may be out of date or named differently than the ones you use in CI. Instead, it's better to leverage pre-commit's ability to pass changed filenames to commands to Pants, and tell Pants to ignore files it's been told not to track:
However, using `--changed-since=origin/main` is not as useful locally. Developers my have unstaged or untracked files lying around in their working directory, or their Git references may be out of date or named differently than the ones you use in CI. Instead, it's often better to leverage pre-commit's ability to pass changed filenames to commands to Pants, and tell Pants to ignore files it's been told not to track:

Comment on lines +199 to +200
Using with pre-commit
---------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

This means H2, and it makes the H3 for "### Tuning resource consumption (advanced)" in the wrong place. Maybe keep it as H2 but move to lower in the file?

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.

4 participants