lib: don't duplicate nixpkgs' lib
The current implementation of nvf's custom lib uses nixpkgs' lib.extend function.
This is intended as an escape hatch for buggy behaviour in nixpkgs and adds unnecessary eval time.
As well, when I was initially looking at nvf's codebase, the use of the lib.nvim namespace obfuscated where functions were actually coming from in nvf's code, but this might be a nonissue depending on personal opinion.
This PR converts the codebase to a separate extensible fixed-point, implemented similarly to nixpkgs' lib, without being a direct extension of it.
This is fairly simple and imo actually reduces complexity with not having to keep around the extension function. I think it's fair to call this a reduction in technical debt.
The current implementation is incomplete, since I want to know whether this might be accepted and, if so, whether to take the approach:
- With a minimal diff that keeps the reduced eval time and prevents any the issues nixpkgs warns against, but doesn't improve code clarity (by separating the two libs explicitly).
This means
_module.args.lib = pkgs.lib // {nvim = self.lib;}, probably. - Which does all, with treewide changes from
lib.nvim->self.lib.
Sanity Checking
- [ ] I have updated the changelog as per my changes
- This shouldn't change anything for users, since the version exposed by the flake already stripped nixpkgs out.
- [ ] I have tested, and self-reviewed my code
- Reviewed a previous version which worked without issues, want advice before proceeding here and doing more thorough testing.
- [x] My changes fit guidelines found in hacking nvf
- Style and consistency
- [x] I ran Alejandra to format my code (
nix fmt) - [x] My code conforms to the editorconfig configuration of the project
- [x] My changes are consistent with the rest of the codebase (in terms of style)
- [x] I ran Alejandra to format my code (
- If new changes are particularly complex:
- [ ] My code includes comments in particularly complex areas
- [ ] I have added a section in the manual
- [ ] (For breaking changes) I have included a migration guide
- No need for this to be breaking.
- Package(s) built:
- [ ]
.#nix(default package) - [ ]
.#maximal - [ ]
.#docs-html(manual, must build) - [ ]
.#docs-linkcheck(optional, please build if adding links)
- [ ]
- Tested on platform(s)
- [ ]
x86_64-linux - [ ]
aarch64-linux - [ ]
x86_64-darwin - [ ]
aarch64-darwin
- [ ]
Add a :+1: reaction to pull requests you find important.
:rocket: Live preview deployed from 3e48f13c3ce8372d00be2e27f313f2ed8da5bc82
View it here:
Debug Information
Triggered by: alfarelcynthesis
HEAD at: no-extend-lib
Reruns: 1501
(@NotAShelf since I want opinions before marking this ready for (final) review)
To be honest I'm not a huge fan of separating lib and nvf-lib, but I can see why one would want to. I'd like to defer to @horriblename as I'm currently too occupied irl to deal with large-scale refactors myself.
I am completely out of my depth with this, the link gives some very scary warnings but gives no explanation on why, so I'm not really sure what to do with that info
If you can help me understand any of this it would be much appreciated :)
The currently-used nixpkgs.lib.extend function computes lib with the provided overlay function (in stdlib-extended.nix), in the same way a nixpkgs overlay works.
This PR converts the custom library to be created in the same way as lib itself is created, as a fixed point containing only the local library functions.
The warnings they mention probably aren't really relevant, except for if someone using nvf makes their own extended lib and tries to provide it using extraSpecialArgs to the (standalone) module.
I'm honestly not sure if that would produce a reasonable eval error or if it would yell about missing lib.nvim functions (if the user lib overrides the nvf-provided specialArg).
Essentially, if lib.extend (and probably specialArgs/extraSpecialArgs) are used in multiple places it can cause issues, but I'm not sure how important that actually is.
I assume they also warn against modifying the value lib since people expect the variable to behave in a certain way.
The current design doesn't directly punish that assumption (since it only adds things), so it isn't necessarily bad.
- In that vein, in my opinion it would be easier to reason about the project (especially as an occasional contributor) if
libwas same thing as it is everywhere else and custom functionality come fromselfor another argument representing this project. - I also think that sticking to using
libas thenixpkgsmaintainers expect is just a generally good idea, which is why I linked the warning. Sorry if it was confusing, I should have been clearer about that.
Ease of understanding is (mostly) a question of maintainer preference though, so if y'all don't actually see benefits it probably isn't worth doing.
The eval benefits of separating the two library evaluations are untested but probably very slightly positive. That isn't enough to make this worth it on its own, though.
Let me know if I talked about the wrong things or if you want changes/to close this :)
(Sorry it took me a couple weeks to get back to this, I was busy and figured this wasn't particularly urgent)