Skip to content

Conversation

zkat
Copy link

@zkat zkat commented Jan 2, 2025

Fixes: #3891

Starting this off as a draft, so we can discuss the changes and see what needs adjusting. I had to update dependencies a bit in order to resolve some cargo resolution conflicts, and with that came a bump in MSRV, as well as the removal of the deprecated backtrace feature for insta.

This uses kdl-rs' built-in "v1-fallback" system: It will first try to parse a KDL document as if it were v2. If that fails, it will try again with the legacy v1 parser (which is identical to the one Zellij is currently using, so v1 docs will parse the same). If both fail, kdl-rs will try to apply some heuristics to figure out which parser's errors to display, and if the heuristics fail, default to v2 errors. Note: It is completely safe for well-formed v1 or v2 documents to be parsed with either parser. If a document just so happens to parse successfully under both parsers, the returned data will be identical.

Since the new KDL v2 parser also supports multiple diagnostics, I took the liberty of restructuring the KdlError type such that it would support that, too, and integrated it with the new error types from kdl-rs. This change also means that Zellij will be able to return One Big Err with a bunch of diagnostics in it, instead of erroring and exiting on the first failure (DeserializationErrors will already do this--but you will now be able to append errors either to that, or to the post-deserialization semantic checks).

@zkat zkat mentioned this pull request Jan 2, 2025
@imsnif imsnif self-assigned this Jan 2, 2025
@zkat
Copy link
Author

zkat commented Jan 3, 2025

I’ll… figure out what’s going on with the tests soon 😅

might be a fallback issue.

@imsnif
Copy link
Member

imsnif commented Jan 10, 2025

Hey! So - I'm starting to take a look at this.

Right now on this branch, Zellij doesn't start. We get a Deserialization error: Failed to parse KDL document. My guess is because we're failing to deserialize the default layout (I think this is also why most of the tests are failing).

I think an easier example to iterate over would be this test: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/tab/unit/tab_integration_tests.rs#L6567 - in which we're failing to serialize one of these: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/tab/unit/tab_integration_tests.rs#L6574-L6588 and https://github.com/zellij-org/zellij/blob/main/zellij-server/src/tab/unit/tab_integration_tests.rs#L6592-L6608

I'm not too sure why every now and then we get a failure for the 1st and at other times we get a failure for the second. I can try to take a deeper look if something doesn't jump out.

I'd recommend skipping our build system and running it directly inside the zellij-server folder with cargo test -- base_layout_is for quick iteration.

@theAkito
Copy link

Any chance at least partial v2 support may be added? It is particularly mocking to see tons of syntax errors, when editing those KDL files with an up-to-date language server and then getting errors like this from Zellij.

zellij

  × Failed to parse Zellij configuration
   ╭─[/home/user/.config/zellij/config.kdl:1:1]
 1 │ keybinds clear-defaults=#true {
   ·                         ▲
   ·                         ╰── Please refer to https://github.com/kdl-org/kdl/blob/main/SPEC.md#value for valid KDL value syntaxes.
 2 │     locked {
   ╰────
  help: For more information, please see our configuration guide: https://zellij.dev/documentation/configuration.html

While the referenced documentation specifically says, that plain text booleans are not allowed, plus other notes regarding booleans' special treatment.

Would be a huge & immediate DX improvement, if those booleans would be fixed.

@imsnif
Copy link
Member

imsnif commented Jun 24, 2025

While I would definitely want to see KDL v2.0 support in Zellij, the migration is not trivial (nothing to do with KDL and everything to do with infrastructure changes in a large code base being time consuming and difficult). Add to that the admin work that would have to go with it (communicating this to users, migration paths, updating documentation/screencasts/tutorials, etc.) and this becomes a large project with limited impact on the quality of life of users.

Zellij is underfunded and our development resources are stretched thin. I prioritize what I think is right.

I absolutely love KDL and am dying to see V2.0 in Zellij, but I just can't afford to spend the time on it presently.

@zkat
Copy link
Author

zkat commented Jun 24, 2025

I've been kinda crispy and busy with a new job and other life stuff, but if there's anything from the list of things blocking this that you think me or the community can pick up to lighten your load, @imsnif, just lmk. I imagine at least some of those things you would want to end up doing, yourself.

@imsnif
Copy link
Member

imsnif commented Jun 24, 2025

For sure. I think if you or someone from the community would like to make a PR that can parse v2 files as easily as it can v1, I can do the rest. Not for this upcoming version in crosses fingers a few weeks time, but I can make the time for next one.

@zkat
Copy link
Author

zkat commented Jun 24, 2025

isn't that basically what this PR is?

@imsnif
Copy link
Member

imsnif commented Jun 25, 2025

It is, yes! But I think there's still a bit of work to be done here.

@zkat
Copy link
Author

zkat commented Jun 25, 2025

absolutely! Just making sure we're on the right track. And if anyone else wants to help me with this PR, I could use the help, too :)

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.

KDL 2.0.0 support
3 participants