nixvim: expose config for nixvim's standalone mode

[MattSturgeon]

Allow standalone nixvim users to take advantage of stylix by exposing the generated config as config.stylix.targets.nixvim.config.
I'm open to other names; maybe out, build, cfg, module?

This can be passed to the nixvim derivation's extendNixvim function, as described in the option description.

Ideally, this would be documented outside of the home-manager/nixos options page, but IMO this is fine. I guess stylix would still be installed via nixos/hm/etc even if nixvim was a standalone build...

I also considered detecting the environment the option is being used in and producing a more specific example; i.e. use home.packages in the home-manager docs' example. I figured this was overkill, and the example should be clear enough for anyone choosing to use standalone nixvim.

Here's a docs screenshot, for reference

FYI this diff is more readable with whitespace changes hidden.

[trueNAHO]

I successfully tested this PR in my standalone Home Manager configuration, which uses NixVim.

Checking config.programs ? nixvim and options.programs ? nixvim is redundant; the mkIf check had no benefit.

This now matches the pattern used elsewhere.

-- e5c11e9

The pattern originates from 7a57dff.

by exposing the generated config as config.stylix.targets.nixvim.config. I'm open to other names; maybe out, build, cfg, module?

AFAIK, the Nix convention is to interface packages as package and wrapped packages as finalPackage. Since Stylix does not wrap the NixVim config, I assume it is better to omit the final prefix, and keep config.stylix.targets.nixvim.config.

See #415 (comment) for an alternative configuration interface.

Ideally, this would be documented outside of the home-manager/nixos options page, but IMO this is fine.

Adding a Standalone section to the documentation would improve scalability of supporting more standalone programs. However, for simplicity, we should reconsider this choice. @danth, what do you think?

I guess stylix would still be installed via nixos/hm/etc even if nixvim was a standalone build...

I assume the same.

I also considered detecting the environment the option is being used in and producing a more specific example; i.e. use home.packages in the home-manager docs' example. I figured this was overkill, and the example should be clear enough for anyone choosing to use standalone nixvim.

Agreed. Also, runtime environment detection is a non-trivial task.

FYI this diff is more readable with whitespace changes hidden.

Thanks for pointing that out! I did not know GitHub supports this Git feature.

[trueNAHO]

What about inlining config.stylix.targets.nixvim.config, as it is used only once? Consequently, the public interface would change from config.stylix.targets.nixvim.config to config.programs.nixvim.

[MattSturgeon]

Apologies, I'm not sure I follow.

If you mean the read-only option should be named programs.nixvim, then that'll conflict with nixvim's option ("option declared twice" error).

If you mean programs.nixvim should be nested within the final config's value, that wouldn't work because nixvim's standalone config modules does not have the programs.nixvim prefix in its option paths.

If you mean we should do something like this, then sure - personal preference:

{
  options = /* ... */;
  config.programs.nixvim = lib.optionalAttrs (/* ... */) (lib.mkIf {/* ... */});
  config.stylix.targets.nixvim.config = {/* ... */};
}

If you mean stylix.targets.nixvim.config should change to something like stylix.programs.nixvim or stylix.configs.nixvim or stylix.out.nixvim (etc) then I'm open to that.

If you mean something else, would you mind clarifying?

[MattSturgeon]

If by inline, you mean we should remove the read-only option; adding that option is the purpose of this PR.

By extracting the attrset intended for nixvim, we allow (advanced) nixvim users not using nixvim's home-manager/NixOS modules to still apply stylix's config to a standalone nixvim (using the derivation's extendNixvim function)

[trueNAHO]

Apologies, I'm not sure I follow.

Sorry for the imprecise explanation.

If you mean the read-only option should be named programs.nixvim, then that'll conflict with nixvim's option ("option declared twice" error).

If you mean programs.nixvim should be nested within the final config's value, that wouldn't work because nixvim's standalone config modules does not have the programs.nixvim prefix in its option paths.

If you mean we should do something like this, then sure - personal preference:

{
  options = /* ... */;
  config.programs.nixvim = lib.optionalAttrs (/* ... */) (lib.mkIf {/* ... */});
  config.stylix.targets.nixvim.config = {/* ... */};
}

Thanks for the detailed analysis! Based on that, your initial proposal seems to be the right one.

If you mean stylix.targets.nixvim.config should change to something like stylix.programs.nixvim or stylix.configs.nixvim or stylix.out.nixvim (etc) then I'm open to that.

What about interfacing the config under the config.lib.stylix namespace, similar to:

stylix/stylix/palette.nix

Lines 149 to 150 in 1d3826c

	lib.stylix.colors = (base16.mkSchemeAttrs cfg.base16Scheme).override override;
	lib.stylix.scheme = base16.mkSchemeAttrs cfg.base16Scheme;

In that case, config.lib.stylix.targets.nixvim.config sounds good to me. @danth, what do you think?

[MattSturgeon]

EDIT: I misread lib.stylix as stylix.lib 😅

Making the read-only option a stylix.lib suboption would probably be neater. Although, having stylix.configs (a sibling of stylix.lib) would be even better (IMO): e.g. stylix.configs.nixvim. This pattern could then also be applied to other targets as stylix.configs.*.

This makes some sense to me, as it's kinda analogous to the final config attrset, which is passed as a module arg alongside lib, pkgs, options, etc.

I think if we go down this route, then it makes more sense to have it documented separately:
The advantage of having the option under stylix.targets.nixvim was discoverability - anyone looking at the nixvim enable option will stumble on the config option - so if they're grouped elsewhere we'd have to improve discoverability through docs (at a later date).

[MattSturgeon]

Thanks for your in-depth feedback! I'll try and respond inline below:

I successfully tested this PR in my standalone Home Manager configuration, which uses NixVim.

Just to clarify, this PR should have no effect on NixOS/home-manager nixvim installations, even ones using standalone home-manager. Rather it is intended to help users who import a pre-built "standalone" nixvim wrapped neovim package.

Checking config.programs ? nixvim and options.programs ? nixvim is redundant; the mkIf check had no benefit.
This now matches the pattern used elsewhere.
-- e5c11e9

The pattern originates from 7a57dff.

Thanks for linking the actual commit. I did understand the use of optionalAttrs is to ensure the attrset is only evaluated when the relevant option exists, since using mkIf would still evaluate the config to check it's valid even when the condition is false.

by exposing the generated config as config.stylix.targets.nixvim.config. I'm open to other names; maybe out, build, cfg, module?

AFAIK, the Nix convention is to interface packages as package and wrapped packages as finalPackage. Since Stylix does not wrap the NixVim config, I assume it is better to omit the final prefix, and keep config.stylix.targets.nixvim.config.

See #415 (comment) for an alternative configuration interface.

Thanks for your thoughts. See my questions on that thread.

I do like the idea of renaming the read-only option something like stylix.configs.nixvim or stylix.out.nixvim though.

I guess stylix would still be installed via nixos/hm/etc even if nixvim was a standalone build...

I assume the same.

I think for now it's fine to consider this a platform-specific option, since it is exposed via a nixos/hm module.

I also considered detecting the environment the option is being used in and producing a more specific example; i.e. use home.packages in the home-manager docs' example. I figured this was overkill, and the example should be clear enough for anyone choosing to use standalone nixvim.

Agreed. Also, runtime environment detection is a non-trivial task.

Well, it can be done fairly trivially; options ? home should do the trick, although you could come up with something more robust. I just figured it was over-engineering a simple example.

[curtbushko]

Friendly ping to bring these two amazing projects together.

[MattSturgeon]

If there's still interest from the maintainers, I'm happy to rebase resolving conflicts so that you can re-review.

I don't really want to spend time understanding what's changed so I can correctly resolve the conflicts, if there's no interest in reviewing/merging 😉

Let me know if you'd prefer I open another PR, so that you can review fresh without the clutter of outdated comments 🙂