-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Git pre-commit/push hook #221
Comments
Only got five hours of sleep today but I’ll have a go at this soon. An important design decision here: do we care that every commit has correct formatting, or only that the state of the final HEAD being pushed does? The former matches what a I personally prefer having CI green after every commit when possible for the purpose of bisecting, but for formatting the downside of having incorrectly‐formatted intermediate commits may be minor, it would add additional fuss for users having to potentially amend multiple commits, and it would mean that no intermediate commit could ever have invalid Nix syntax (which, to be fair, might be a good thing). |
@emilazy CI will only care about HEAD being properly formatted (or rather, all files that changed between the base branch and HEAD), but I don't think it should be a pre-push hook, because it would require an extra commit to reformat all files changed in all of the commits, ending up with cases like this: gitGraph
commit id:"Change file A"
commit id:"Change file B"
commit id:"Reformat file A and B"
The last commit should then arguably even be added to So I think a pre-commit hook would be better, it keeps the history cleaner and as you say would also enforce correct Nix syntax for each commit, which I also think is a good thing :) |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-07-23/49510/1 |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/satisfaction-survey-from-the-new-rfc-166-formatting/49758/37 |
I generally dislike the use of pre-commit hooks and I like that nixpkgs doesn't have any such hooks until now. The code not being formatted and being flagged by CI would bring up issues with the author's environment setup. Adding a safety net for it (hooks) would mean that those environments remain mis-configured. What I'm suggesting would allow for some friction to remain but I think it's worth promoting better configuration of dev environments. What do you think? |
if i understand it right, there's only really a handful of ways to ensure every developer's environment results in nixfmt-conforming formatting:
don't underestimate how varied dev environments are w.r.t. editors. there may be value with a contrib/ folder to share editor-specific nixfmt configs, Home Manager integrations, nixpkgs git as a point of integration is likely to reach almost every nix dev. i think most editors integrating git do so in a way that will trigger the commit hooks (might be worth confirming that)? you still won't reach That Guy who manages his nixpkgs repo with fossil and only exports to git for the PR process but, well, maybe it's worth having both of these things then. |
AFAIK it's Git that executes the pre-commit hooks, not the editor; so, this should be fine.
Given you're referring to the hooks and the CI as "both", the question of having it on the CI is a definite yes, because having a check as a hook and not on the CI is like having front-end validation but not back-end ones. To clarify, I wasn't recommending a different ways of ensuring that each developer environment is accurate; rather I'm suggesting to not ensure that at all. Mainly have 2 reasons for this:
Do let me know why you think this doesn't sound worth it, if at all. |
i was saying commit hook + editor integrations are both worth having.
this is exactly what we have to avoid. most people do not care about nix code formatting. i do not mean that in a disparaging way: for any particular nix development (a new CLI option, a new package or service, etc), most users do not benefit and do not care. development progresses only because those who care about a thing go about integrating it in a way that doesn't negatively impact those who don't care about it. formatting is no exception. the ideal integration would be that nobody has to adjust any config. as you point out, "nobody has to adjust any config" is in tension with "no miscellaneous code is being run on device". oh well, we can at least add an easy-to-toggle and easy-to-discover option and gauge desire for making it a default once it's there. that's what the git hook gets us. i'm vague on what else you mean by "environment", except editor hooks or shell hooks or something like that. those could be worth having, but they're not the same type of single-point-of-integration as these git hooks. you can't have the CI failure say "add |
Hmm. You're right that we probably shouldn't push for something that might scare away contributors, who are the best resource we have. In which case, contradictory to my own point, I think it tips the scale in favour of "nobody has to adjust any config". My concerns were more general and TBH I'd be willing to make an exception for the general good. (not that my personal concerns would've stopped this from going through, but ykwim) |
You can make hooks suggested but ultimately optional by providing basic examples. An example would be creating a To use our suggested Git VCS hooks, run from the project root $ git config --local core.hooksPath .githooks This allows the user to easily get & use said hooks while trusting the makers/maintainers to not push something shady in these hooks, or they are free to modify |
We should have this in Nixpkgs for NixOS/nixpkgs#326407. Here are some notes for that:
git filter-branch
would be much simpler than that script thoughnixfmt
version pinned in theshell.nix
.git/hooks
automatically viashellHook
(this is also how https://github.com/cachix/git-hooks.nix works)Ping @emilazy
The text was updated successfully, but these errors were encountered: