Skip to content

Disallow commits to main via pre-commit-hooks #9248

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

Closed
wants to merge 4 commits into from

Conversation

headtr1ck
Copy link
Collaborator

@max-sixty
Copy link
Collaborator

LGTM. I have a tiny bit of pause that someone just starting out in git is going to get really confused, but that's quite minor...

@max-sixty
Copy link
Collaborator

(Also to confirm — this won't fail when run on main outside of a commit, in particular on CI?)

@keewis
Copy link
Collaborator

keewis commented Jul 15, 2024

I guess while this can be confusing, it is also partly a blessing: especially people just starting out will open a pull request from main and then run into all sorts of issues. Though in my experience people starting out don't tend to use pre-commit, so this might not be an issue.

@headtr1ck
Copy link
Collaborator Author

(Also to confirm — this won't fail when run on main outside of a commit, in particular on CI?)

I think it will fail... The hook has no way of knowing why it was invoked.
Although, we could change it into a pre-push stage?
I just find it better to fail before committing, so you don't have to clean up your local main history later...

@keewis
Copy link
Collaborator

keewis commented Jul 15, 2024

does pre-commit.ci even run on main? Edit: yes, it does. Though as a push event instead of a pr event.

This might still be useful, but I think for now this might be resolved by splitting up the branch protection rule.

@headtr1ck
Copy link
Collaborator Author

does pre-commit.ci even run on main? Edit: yes, it does. Though as a push even instead of a pr event.

Now it does not anymore ;)

@max-sixty
Copy link
Collaborator

does pre-commit.ci even run on main? Edit: yes, it does. Though as a push even instead of a pr event.

Now it does not anymore ;)

Presumably this precludes us from using this?

Same if someone run pre-commit run -a on an up-to-date main locally — it's important that passes...

@headtr1ck
Copy link
Collaborator Author

Presumably this precludes us from using this?

why? Now it only runs locally.

Same if someone run pre-commit run -a on an up-to-date main locally — it's important that passes...

Not sure why this has to pass, or why someone would even run that.

But hey, no strong feelings about this!

@max-sixty
Copy link
Collaborator

max-sixty commented Jul 15, 2024

[deleted — previous version was incorrect]

Not sure why this has to pass

I often run some form of pytest && pre-commit run -a, and I would want that to pass on the current best version. I don't use git bisect much but similarly I think the principle of "tests always pass on main" is good.

But very happy to defer to whatever others think. It's also easy to change — I would encourage more change and fewer vetos!

@max-sixty
Copy link
Collaborator

Unless there are supporters, I would vote to close :'(

@max-sixty max-sixty added the plan to close May be closeable, needs more eyeballs label Sep 11, 2024
@headtr1ck headtr1ck closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to close May be closeable, needs more eyeballs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants