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

Extend configuration #28

Closed
wants to merge 2 commits into from
Closed

Extend configuration #28

wants to merge 2 commits into from

Conversation

andrevmatos
Copy link

Fixes #23

Gives neovim packages a extendConfiguration property, which is a method allowing users to extend this package's configurations with additional modules.

This allows users e.g. to derive .#nix config to generate their own config, assign that to global neovim package, and then in per-project's shell.nix, expand the config to include additional LSP modules or plugins, instead of having to redefine everything from neovimConfiguration.

anywhere `neovim` package is exposed by this flake, a new property,
`extendConfiguration` is made available to allow extending previous
configurations/modules in place.
This allows e.g. a global package with minimal LSP support to contain
users configurations, and per-project shell.nix to enable the language
servers needed by that project. Also, allow users to more easily extend
.#nix and .#maximal packages to include their customizations.
Modules "default" configs should be set with `mkOptionDefault`, allowing
users to use `mkDefault` or others to set their options, which then can
be customized downstream with `neovim.extendConfiguration`.
`mainConfig` also receives an `overridable` value of 1200, allowing
user's `mkDefault=1000` to override it while overriding modules's
`mkOptionDefault=1500` properly.
Copy link
Owner

@jordanisaacs jordanisaacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm except for the small nit. Sorry for the time taken to review this. Could you also add a note to the changelog & give an example of using this in the Custom Configuration section

check = args.check or check;
extraSpecialArgs = extraSpecialArgs // (args.extraSpecialArgs or {});
})
.neovim;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.neovim;

From my understanding, the result of the function does not require being .neovim. I would rather the output of extendConfiguration be identical to neovimConfiguration (as the module exports some useful information).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, my ide ahere is that neovimConfiguration is already exported as a function, and since this property would be come a method in neovim, it'd make sense that it also returned another neovim, to be used in place; maybe we could have a top-level extendConfiguration returning the whole set, and the property in neovim returning the package directly?

Copy link
Owner

@jordanisaacs jordanisaacs Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you rebase this PR onto master it solves itself. You can make extendConfiguration a passthru here and just do a standard module eval. No need to use the neovimConfiguration funciton. See here for accessing the special values.

Copy link
Owner

@jordanisaacs jordanisaacs Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do not have the time for figuring out the changes, I can take this PR over. But I do not want to step on your work as this is a great solution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, feel free to take it over; 10mo baby here is requiring additional time this week, and I won't have time to implement for a couple of days. I'm happy to have contributed at least the idea, but of course you as creator has the best big picture view to make it right. Thanks

@jordanisaacs jordanisaacs mentioned this pull request Apr 5, 2023
17 tasks
jordanisaacs added a commit that referenced this pull request Apr 10, 2023
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.

Custom Configuration based on maximal, etc.
2 participants