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

Fix pre-commit documentation #200

Merged

Conversation

schuellerf
Copy link
Contributor

Two minor things.

The name of the revision has to be exactly the same as the git tag, I think. Which (at least until 2.10) is without the "v"?

pass_filenames is already part of .pre-commit-hooks.yaml so I guess this is obsolete here?

@gir-bot gir-bot added S: needs-review Needs to be reviewed and/or approved. C: docs Related to documentation. labels Jan 15, 2025
@schuellerf schuellerf changed the title Fix pre commit documentation Fix pre-commit documentation Jan 15, 2025
@facelessuser
Copy link
Owner

Yes, I've never used the v prefix on this project. I believe that was a PR from a 3rd party and I missed it.

@facelessuser
Copy link
Owner

I see your change also removes pass_filenames: false. Is there a reason?

@facelessuser
Copy link
Owner

Oh, I guess you commented on pass_filenames: false. I'm really not sure. I haven't actually used the hook before 😅. Again, the hook and documentation was committed by a 3rd party.

@schuellerf
Copy link
Contributor Author

schuellerf commented Jan 15, 2025

I was expecting this to be a minor "typo" anyway ;)

@facelessuser
Copy link
Owner

@sscheib can you comment on the changes? I believe you originally committed the change?

@sscheib
Copy link
Contributor

sscheib commented Jan 15, 2025

that was indeed an oversight on my end. @schuellerf is correct with the PR. at the time back then there was no new release, which is why I tagged in for my use with the commit hash.

Sorry for the inconvenience!

@facelessuser
Copy link
Owner

@sscheib What about the removal of pass_filenames: false in the docs?

@sscheib
Copy link
Contributor

sscheib commented Jan 15, 2025

it's not strictly necessary.
the reason I added it to the documentation was to ensure that nobody accidentally sets it to true, just because that person has been setting it for other hooks in the past.
setting it to true would cause pyspelling to fail and might increase the issue of maintenance (due to increased issues getting created for no reason)

@facelessuser
Copy link
Owner

Then it's probably worth leaving in the documentation or adding a warning of removing.

@schuellerf schuellerf force-pushed the fix-pre-commit-documentation branch from f41e767 to 189025e Compare January 15, 2025 18:31
@schuellerf
Copy link
Contributor Author

It just felt unnecessary and lengthens the config, but it's ok to just leave it in

@facelessuser
Copy link
Owner

@gir-bot lgtm

@gir-bot gir-bot added S: approved The pull request is ready to be merged. and removed S: needs-review Needs to be reviewed and/or approved. labels Jan 15, 2025
@facelessuser facelessuser merged commit 4588bb0 into facelessuser:master Jan 15, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: docs Related to documentation. S: approved The pull request is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants