-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
formats.kdl: init #295211
base: master
Are you sure you want to change the base?
formats.kdl: init #295211
Conversation
4b0381f
to
d602f63
Compare
d602f63
to
93d71af
Compare
93d71af
to
16562ac
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f97ae1b
to
7fc1115
Compare
7fc1115
to
f7a3cc5
Compare
How come we're depending on an external application to convert to kdl? It's fairly trivial to do in pure nix - and this has the advantage of letting you use the serialized kdl document within nix code without requiring import-from-derivation. I know this, because i maintain a module for an application with a kdl config, and i use this approach to generate the kdl document: https://github.com/sodiboo/niri-flake/blob/fcc9ec28393f76d269a58db18f6c48595a0da56d/kdl.nix Note that my implementation makes a couple opinionated decisions that may be somewhat unfit for nixpkgs (since i'm only catering to one application). But the actual serialization should be just about the same, though, as the internal representation is nearly identical. What about type annotations? (note that in my case, the application i target explicitly rejects any documents with type annotations, so my implementation pretends they do not exist) @AgathaSorceress mentions in a previous comment that they couldn't represent comments or type annotations in a good way. I conjecture, that it shouldn't be too hard to represent them in a way that we can serialize: simply replace all Comments are much less important, given that they are not significant for KDL's data model, but if we must support slashdash comments, we could totally have some attribute like |
I agree that KDL could very well be serialized entirely in Nix, and to be honest, that would be my personally preferred method too. However, this implementation heavily follows the existing standards already set up by other formats in If a purely Nix based implementation is preferred, I would be up for helping to implement this (though it seems you (@sodiboo) do already have a pretty good working implementation), however this is my first nixpkgs PR so I'm unsure who is fit to "decide" this. Regarding type annotations, I agree that they would be easy to implement, and it would definitely make sense to integrate them. I will try to do that once I have some free time in the upcoming days. I do honestly think that being able to generate comments is just not important for a language-to-language serializer, since comments do not (or at least should not) have semantic meaning, and the semantic meaning is the main focus of the document here. In my opinion, this is out of scope. |
f7a3cc5
to
b423ba0
Compare
I did not realize the other formats also generate derivations, since i'm not too intimate with I suppose, there's not much of a use case for manipulating these formats after the fact (if you need to parse -- just don't serialize in the first place), but i do think it would be necessary for e.g. embedding one format within another. That's probably somewhat of an esoteric use case though, and i don't know of any tooling that actually wants this. Ultimately, it seems Given how |
b423ba0
to
bc34289
Compare
Any blockers? |
The only remaining blocker is the type annotation support mentioned in #295211 (comment) that I never got around to implementing. This should be a quick fix, but could technically also be implemented in a follow-up PR in order for this PR to be merged sooner. |
bf5e571
to
9c463e5
Compare
Type annotation support has been added now, and seems to work fine based on initial tests. From my point of view, this PR is now ready to merge if there are no other issues. |
9c463e5
to
dd18e95
Compare
dd18e95
to
c882b47
Compare
Thank you for the great work @feathecutie. I have been thinking about merging and overriding. Considering nix attrsets are completely orderless and KDL nodes are order-sensitive, representing them as lists are the only real choice. But being able to override previously set values is one of the great strengths of the module system IMO. But this doesn't really work that well with lists. Do you think it would be possible to create a type for the format where you could optionally use attrsets instead of lists (for applications that does not care about order), and defer type coercion until generation or until in needs to merge with something that is already a list? I'm hoping this would let the user override freeform KDL configuration. I suppose this would not be a breaking change from the existing code, so we could merge this first and then go about extending it in another PR. But I'm interested to hear your thoughts, since you're the author of the current implementation. |
@sodiboo just for some context, there has been a bit of pushback on large generators written purely in nix before. I believe the main concern is evaluation speed, coupled with some other things: #208747 (comment). But I don't think there's a clear consensus |
That sounds like a good idea, I'm just not sure exactly what would be the best way to implement that. Did you have any specific format of attrset in mind? One option that would feel "natural" in my opinion would be to simply have the names of nodes be the keys of the attrset, but even disregarding order, that would still not work because of the fact that KDL allows multiple nodes with the same name. I'm also not entirely sure if this is actually the correct level to implement merging at, since the main job of this format is just to somehow translate Nix to KDL, and KDL simply does not have a generalizable concept of "merging" configurations. Depending on how KDL is used in each specific module or usecase, it might very well be possible that configurations should be merged differently (e.g. simply concatenating all lists, or replacing all nodes of a specific name, or something else entirely). In these cases, it might make more sense to add custom options that do not directly translate to I also realize that some modules might simply wish to use |
Description of changes
Adds
pkgs.formats.kdl
, a Nix representation of the KDL document language using the standardpkgs.formats
format.Also inits
json2kdl
, a custom-built application which is required for the core functionality ofpkgs.formats.kdl
.Closes #198655.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.