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

feat(config): add KDL v2 support #3908

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat(config): add KDL v2 support #3908

wants to merge 1 commit into from

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.

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
2 participants