-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add scripts/nixfmt-mergetool
#277
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat, I didn't know about git mergetool
! While this way of merging isn't fool-proof against syntactic errors, it is quicker than the auto-rebase script, and there's no problem with having both, so I say let's have this. Will need some work to integrate into the build, but we can do this in follow-up PRs.
For reference, here's an example of a syntactic error that can be introduced. Consider this file:
{
foo
# , bar
}:
null
Formatting this on one branch turns it into
{
foo,
# , bar
}:
null
But if on another branch the extra argument gets uncommented:
{
foo
, bar
}:
null
You'll end up with this invalid merge result (no merge conflict):
{
foo,
, bar
}:
null
This is an easily catchable syntactic error though. I can't imagine such a failure changing semantics, but would love to be proven wrong.
# Copyright (c) 2025 Jan Malakhovski <[email protected]> | ||
# | ||
# This file can be redistributed under the terms of Unlicense | ||
# <https://unlicense.org> license. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only worry I have is the re-introduction of separately licensed files, which we moved away from in the past: #171. Would you be okay with you to re-license it under the MPL-2.0 license and assign copyright to the generic "Nixfmt contributors" as defined in
Lines 3 to 6 in e825e95
Files: * | |
Copyright: 2019-2022 Serokell <[email protected]> | |
2019-2024 Nixfmt Contributors | |
License: MPL-2.0 |
# Copyright (c) 2025 Jan Malakhovski <[email protected]> | |
# | |
# This file can be redistributed under the terms of Unlicense | |
# <https://unlicense.org> license. |
Inspired by oxij's #277, but implemented in Haskell, therefore faster (less process spawning) and trivially integrated into the build and distribution.
Because it's a bit tedious to distribute a shell script along with the build, I made a PR to implement this directly in the Haskell codebase: #283. This also benefits from improved speed, because three less processes need to be spawned. |
On license: yes, feel free to re-license under whatever, I selected unlicense exactly for the reason it allows doing that.
On builtin `--mergetool`: nice!
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2025-02-18/60511/1 |
Also just opened numtide/treefmt#523, because this functionality would be useful for all formatters :) |
Makes resolving formatting-related conflicts easier. Idea and docs are from @oxij's #277. Code and tests are from @infinisil. By implementing it in Haskell, it's faster (less process spawning) and trivially integrated into build and distribution. Co-Authored-By: Jan Malakhovski <[email protected]>
Makes resolving formatting-related conflicts easier. Idea and docs are from @oxij's #277. Code and tests are from @infinisil. By implementing it in Haskell, it's faster (less process spawning) and trivially integrated into build and distribution. Co-Authored-By: Jan Malakhovski <[email protected]>
Done in #283 |
The script is self-documenting.
IMHO, this script should also be added to the output of
nixfmt-rfc-style
of Nixpkgs. Otherwise, after recent treewide changes, things became impossible to merge manually.