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

generators: new toKDL parser #4614

Closed
wants to merge 6 commits into from
Closed

generators: new toKDL parser #4614

wants to merge 6 commits into from

Conversation

cgahr
Copy link

@cgahr cgahr commented Oct 29, 2023

Description

My idea for a new toKDL parser. The current one has significant problems since it is not possible to encode all possible KDls in nix (see #4613 #4612 #4485 #4465 #4160 #4054).

I propose this PR as a solution.

Idea

In its core, KDL is a language encoding not only data but also lists of sequential commands. By its nature, nix has problems encoding this.

In addition, the current implementation of toKDL was inspired by the rules for converting kdl to json. This, however, is the wrong direction: we want to convert nix -> json -> kdl.

As such, the current implementation cannot encode things like

layout {
  pane
  pane
}

Implementation

The idea is the following:

  • parse the settings recursively
  • lists are converted to lists of strings to be concatenated
  • sets are also converted to lists of strings
  • every basic datatype is converted to a string and added to the list.
  • Lists of lists and lists of sets are automatically flattened

Incompatibilities

List of literals

All lists and sets are "exploded", i.e. each entry gets its own line:

{
  layout = [ pane pane ];
}

before:

layout pane pane

now:

layout {
  pane
  pane
}

In this case the previous behavior was intended to make writing the config easier but made the previous example impossible to express.

_args and _props

_args and _props do not work anymore:

{
  bind = {
    _args = [ "a" ];
    _props = { maybe = true };
}

Before:

bind "a" maybe=true

Now:

bind {
  "a"
  maybe = true
}

This behavior can be readded. Afaik, it was only intended to be a workaround to handle nix limited capabilites. With this new parser, they are not needed anymore. We could consider properly depcrecating this behavior.

What's new

Converting ' to "

The new parser heavily uses strings. To make to syntax easier I propose to convert ' automatically to '"' if they are in the name of a set or in a list:

{
  "bind 'a'" = { "something" }
  "bind \"b\"" = [ "something" ]
  "bind 'c'" = { "something" = "don't convert here" }
}

Now:

bind "a" {
  something
}
bind "b" {
  something
}
bind "c" {
  something "don't convert here"
}

This is technically incompatible with the kdl specs since the following is a valid config:

can't be right "right?"

This would be impossible to write as a nix with my proposed parser.

much more strings

With this parser much more strings would have to be used. Consider the following nix config for zellij:

{
  shared = [
    "unbind 'Ctrl p'"
    "unbind 'Ctrl n'"
    "unbind 'Ctrl s'"
    "unbind 'Ctrl o'"
    "unbind 'Ctrl t'"
    "unbind 'Ctrl h'"
    "unbind 'Ctrl b'"
  ];

  locked = {
    "bind \"Alt h\"" = { MoveFocusOrTab = "Left"; };
    "bind \"Alt l\"" = { MoveFocusOrTab = "Right"; };
    "bind \"Alt j\"" = { MoveFocus = "Down"; };
    "bind \"Alt k\"" = { MoveFocus = "Up"; };
  };

  pane = {
    "bind \"t\"" = [ "CloseTab" "CloseFocus" ];
  };

  normal = {
    "bind 'p'" = { SwitchToMode = "Pane"; };
    "bind 'n'" = { SwitchToMode = "Resize"; };
    "bind 's'" = { SwitchToMode = "Scroll"; };
    "bind 'o'" = { SwitchToMode = "Session"; };
    "bind 't'" = { SwitchToMode = "Tab"; };
    "bind 'h'" = { SwitchToMode = "Move"; };
    "bind 'b'" = { SwitchToMode = "Tmux"; };
  };
};

In my opinion, this is much more readable compared to before (and of course it is now possible to encode things that were not possible before).

Checklist

For now the tests are failing, the original tests are falling to have a reference to how it used to be.

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@emilazy @foo-dogsquared @erikkrieg @h7x4 @rycee

What do you think?

@h7x4
Copy link
Contributor

h7x4 commented Oct 30, 2023

Hello @cgahr!

Thank you for looking into this. I strongly agree with you that the KDL parser is flawed and needs a complete rewrite. It probably shouldn't have been merged in the first place, and more work should have gone into ensuring a more complete one to one correspondence between the generator API and KDL.

I'm currently working on a new set of generators for more obscure config languages in nixpkgs. Starting with libconfig at NixOS/nixpkgs#246115, I'm working my way through HOCON and KDL right now, using a very similar nix -> json -> rust -> config based pipeline, with plans of doing RON and Dotconf at some point in the future. The reason behind the rust approach is that the previous libconfig PR, which was of comparable size with this, was staled/rejected due to nix evaluation time overhead. There are also a bunch of other comments there you might find interesting or useful.

I think the KDL parser is much more needed in home-manager right now due to the broken state of the zellij module, but completing this also means potentially large duplicated efforts. If you'd like to join me, I'd be happy to work together on this! Feel free to DM me on matrix. If you decide you'd rather work on completing this here for now, I'll be happy to review and test the code.

@patwid patwid mentioned this pull request Nov 14, 2023
5 tasks
Copy link

stale bot commented Jan 28, 2024

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants