michidk / vscli

A CLI/TUI that simplifies launching VSCode projects, with a focus on dev containers
https://crates.io/crates/vscli
MIT License
130 stars 4 forks source link

fix: allow trailing commas in devcontainer.json file #95

Closed popsu closed 6 months ago

popsu commented 6 months ago

The current version doesn't like trailing commas in devcontainer.json file:

$ vscli open                                                                                                     
Error:                                                                                                           
   0: Failed to parse json file: "/home/popsu/prepos/test-repo/.devcontainer/devcontainer.json"                
   1: trailing comma at line 24 column 4                                                                         

Location:                                                                                                        
   /home/popsu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/vscli-0.2.2/src/workspace.rs:65              

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.                                 
Run with RUST_BACKTRACE=full to include source snippets.   

This PR changes the serde_jsonc to serde_jsonrc package allows the trailing commas.

https://docs.rs/serde_jsonrc/latest/serde_jsonrc/

michidk commented 6 months ago

Thanks for this improvement! Should we maybe straight up switch to https://docs.rs/json5/latest/json5/?

popsu commented 6 months ago

Should we maybe straight up switch to https://docs.rs/json5/latest/json5/?

I'm not that familiar which JSONC crates one should use in Rust. I found the serde_jsonrc by a little bit of googling, and it seems to work as a straight replacement.

I checked this json5 crate now that you mentioned it. Based on the docs it only implements from_str and to_string functions. So it can't be used as straight replacement, but some refactoring would be needed. Since it would be more work, there would have to be some upside to use this crate instead. With my limited knowledge about these I can't see any upside. I'm always open to learning, so let me know if there is some upside.

popsu commented 6 months ago

I'm probably missing something about how to use Serde with custom serializer/deserializer? :thinking:

Since the json5 implements serialize and deserialize, there should be a way to plug that somehow in to Serde, so we can use all these functions used in this vscli without having to refactor any code? I just don't know how to.

popsu commented 6 months ago

Looking at the docs (second example) of json5, seems that we could deserialize using json5 into serde_json::Value and then use serde_json functions after that.

michidk commented 6 months ago

Exactly.

You can just replace serde_jsonc with serde and add json5. You would then have to adjust parse_dev_container_config to do it like this:

let content = std::fs::read_to_string(path)?;
let config: serde_json::Value = json5::from_str(&content)
michidk commented 6 months ago

Then we would get finally rid of serde_jsonc which is kind of unofficial. You could also add some simple unit tests to verify that tailing command and comments are not an issue.

popsu commented 6 months ago

I changed it to use a combination of serde_json and json5 like you suggested, and I also added unit test to check that deserialization works for a devcontainer.json file that has comments and trailing commas.