Skip to content
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

modules/output: cleanup + refactoring #1889

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MattSturgeon
Copy link
Member

Summary

General cleanup and refactoring to the output module, with the goal of streamlining and simplifying the implementation.

For example, the files submodule has path and plugin options that are essentially the same as extraFiles' target and the top-level's initPath options respectively.

As another example, the content option is currently read-only, meaning we cannot reflect the entire config file content there when we add neovimConfig.neovimRcContent to it.

Being able to do this also means the common (non top-level) modules can handle writing the config file, reducing the complexity in both the files submodule and the top-level output module.

As the changes in this PR are fairly broad, I also added some comments and inlined some sections of the top-level output module. IMO this makes it much easier to understand.

Generalising many of these output options allows for other changes like enabling type="vim" at the top-level (if users configure target="init.vim")

I did make the type option read-only, since it didn't make sense to me for it to default to the target's extension otherwise. But I'm unsure if this is a good change.

Draft

This is a draft because it's currently difficult to test; #1878

It also feels kinda broad, maybe it could be split up into smaller PRs? This is difficult since all the changes touch the same code...

I'd still appreciate feedback on the overall direction, style, changes while we wait for buildbot to work again.

pkgs.wrapNeovimUnstable lua support

While nixpkgs added luaRC support recently, I don't think we can take advantage of that for two reasons:

  1. the wrapper function expects the string content
  2. the wrapper will want to write the init file(s) itself
  3. the wrapper may not order the rc sections the way we would like
  4. the wrapper can't implement wrapRc the way we would like

Related work

It'd be nice to make "standalone" builds less special; I think we can achieve that by adding a standalonePackage option, similar to finalPackage.

I think the "disabling env config" is somewhat unrelated to wrapRc, so perhaps that should have its own option?
I don't think we need wrapRc = mkForce true in the nixos wrapper, other than to ensure /etc is used instead of ~/.config.
Removing "site" from the rtp could also conflict with treesitter (#1855).

Changes when opening PR

  • modules/output: rename path -> target
  • modules/output: introduce finalConfig option
  • modules/output: override content in top-level
  • modules/output: cleanup top-level
  • modules/output: make content type lines
  • lib/utils: move hasContent to helpers lib
  • modules/output: only print relevant parts of config
  • modules/output: allow lua or vim init files
  • modules/output: make type option readOnly

Attempt to be consistent with `extraFiles` and other file types.
Replaces the `plugin` option used in files submodules and the `initPath` option used at the top-level.
General cleanup, in-lining and comments.
This allows us to prefix the RC content built by `makeNeovimConfig`.
This is a breaking change, but IMO it makes more sense to configure this
via the `target` filename.

Either `target` or `type` should be readonly otherwise we would want
circular defaults.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant