danth / stylix

System-wide colorscheming and typography for NixOS
https://stylix.danth.me/
MIT License
902 stars 105 forks source link

nixvim: expose config for nixvim's standalone mode #415

Open MattSturgeon opened 3 weeks ago

MattSturgeon commented 3 weeks ago

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

![image](https://github.com/danth/stylix/assets/5046562/cdd339e5-e2b3-4daf-a88e-7989e9326687)

FYI this diff is more readable with whitespace changes hidden.

MattSturgeon commented 3 weeks ago

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.