-
-
Notifications
You must be signed in to change notification settings - Fork 2k
delta: refactor into standalone module #7134
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
base: master
Are you sure you want to change the base?
delta: refactor into standalone module #7134
Conversation
Running TODO list:
|
3f34b32
to
00088b5
Compare
00088b5
to
a9665ff
Compare
a9665ff
to
1fdb7a9
Compare
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.
I agree that delta
(and other pagers) should have its (their) own module. Good first step
I do have a nit to review for the module options.
enable = mkEnableOption "" // { | ||
default = cfg.enableGitIntegration; | ||
description = '' | ||
Whether to enable the {command}`delta` syntax highlighter. | ||
See <https://github.com/dandavison/delta>. | ||
''; | ||
}; | ||
|
||
enableGitIntegration = mkEnableOption "" // { | ||
description = '' | ||
Whether to enable Git integration for the {command}`delta` syntax highlighter. | ||
See <https://github.com/dandavison/delta>. | ||
''; |
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.
IMO this should default to false
for enable
and true
for enableGitIntegration
.
enable = mkEnableOption "" // { | |
default = cfg.enableGitIntegration; | |
description = '' | |
Whether to enable the {command}`delta` syntax highlighter. | |
See <https://github.com/dandavison/delta>. | |
''; | |
}; | |
enableGitIntegration = mkEnableOption "" // { | |
description = '' | |
Whether to enable Git integration for the {command}`delta` syntax highlighter. | |
See <https://github.com/dandavison/delta>. | |
''; | |
enable = mkEnableOption "" // { | |
description = '' | |
Whether to enable the {command}`delta` syntax highlighter. | |
See <https://github.com/dandavison/delta>. | |
''; | |
}; | |
enableGitIntegration = mkEnableOption "" // { | |
default = true; | |
description = '' | |
Whether to enable Git integration for the {command}`delta` syntax highlighter. | |
See <https://github.com/dandavison/delta>. | |
''; |
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.
Your suggestion seems reasonable to me, at the expense of new users needing to explicitly disable Git integration when adding delta
to their config. That should be fine, as I'd expect a majority of users to want Git integration enabled, anyway. I'll look at implementing it!
Ideally, programs.git.delta.enable
would be split-renamed into both programs.delta.enable
and programs.delta.enableGitIntegration
. We could then keep the default as false
for both new options.
(mkRenamedOptionModule | ||
[ "programs" "git" "delta" "enable" ] | ||
[ "programs" "delta" "enableGitIntegration" ] | ||
) |
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.
Related to my previous comment, this would need to use a custom message with mkRemovedOptionModule
or mkChangedOptionModule
to point at both programs.delta.{enable,enableGitIntegration}
.
finalPackage = | ||
if cfg.settings == { } then | ||
cfg.package | ||
else | ||
(pkgs.writeShellApplication { | ||
name = "delta"; | ||
runtimeInputs = [ cfg.package ]; | ||
text = '' | ||
exec ${getExe cfg.package} --config=${deltaConfig} "$@" | ||
''; | ||
}); |
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.
Makes me wish that dandavison/delta#1959 had been better received.
Description
WIP
Refactor
delta
into its own standalone module, with a newenableGitIntegration
option.Many submodules under the Git module deserve to be broken out into their own spaces. Delta is a great example, as it can be used without Git.
Checklist
Change is backwards compatible.
Code formatted with
nix fmt
ornix-shell -p treefmt nixfmt-rfc-style keep-sorted --run treefmt
.Code tested through
nix-shell --pure tests -A run.all
or
nix build --reference-lock-file flake.lock ./tests#test-all
using Flakes.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
Maintainer CC
@khaneliman
@rycee