surrealdb / surrealist

Surrealist is the ultimate way to visually manage your SurrealDB database
https://surrealist.app/
MIT License
1.03k stars 73 forks source link

feature: Pretty print using serde_json correctly #292

Closed winksaville closed 1 month ago

winksaville commented 1 month ago

It is necessary to use serde_json::from_str to convert the config str into a serde_json::Value then use serde_json::to_string_pretty to get a proper pretty printed result.

This costs an additional line of code and an unknown performance difference, but it does reduce the number of dependencies by 3 removing getopts, jsonxf and unicode-width. I also results in a very slight reduction in the release binary. For jsonxf the size was 27,679,424 compared to serde_json at 27,661,224. Which is 18,200 bytes but as a percentage it's tiny at (18200/27661224)*100 = 0.0658%.

Here is the data.

$ find . -type f -name 'surrealist' -print0 | xargs -0 stat --format="%s %n"
265382360 ./src-tauri/target-main-using-jsonxf/debug/surrealist
27679424 ./src-tauri/target-main-using-jsonxf/release/bundle/appimage_deb/data/usr/bin/surrealist
27679424 ./src-tauri/target-main-using-jsonxf/release/bundle/appimage/surrealist.AppDir/usr/bin/surrealist
27679424 ./src-tauri/target-main-using-jsonxf/release/bundle/deb/surrealist_2.0.6_amd64/data/usr/bin/surrealist
27679424 ./src-tauri/target-main-using-jsonxf/release/surrealist
265040760 ./src-tauri/target-pretty-print-using-serde_json-correctly/debug/surrealist
27661224 ./src-tauri/target-pretty-print-using-serde_json-correctly/release/bundle/appimage_deb/data/usr/bin/surrealist
27661224 ./src-tauri/target-pretty-print-using-serde_json-correctly/release/bundle/appimage/surrealist.AppDir/usr/bin/surrealist
27661224 ./src-tauri/target-pretty-print-using-serde_json-correctly/release/bundle/deb/surrealist_2.0.6_amd64/data/usr/bin/surrealist
27661224 ./src-tauri/target-pretty-print-using-serde_json-correctly/release/surrealist

I also visually verified the two config.json outputs, the jsonxf seems to preserve the order of the original string where as the conversion from a string to a json value then back to a string, as done by serde_json, definitely does not. Finally, I did a quick verification that the jsonxf and serde_json versions could process each other config.json files. The one thing I didn't do was measure performance differences.

Two other data points:

Therefore I'd choose serde_json.

macjuul commented 1 month ago

Looks good to me, and seems like a fair change! Could you run a quick cargo fmt to fix the remaining CI error?

Thanks!

winksaville commented 1 month ago

I'm not sure why it says there is a conflict?

Also, I just noticed there is no linefeed after the trailing brace:

wink@3900x 24-05-24T16:09:48.960Z:~/.config/SurrealDB/Surrealist
$ cat -n config.json
     1  {}wink@3900x 24-05-24T16:09:56.566Z:~/.config/SurrealDB/Surrealist
$ 

Would you like me to append a '\n' to pretty_config?

macjuul commented 1 month ago

I've taken care of appending a newline and fixed the conflict!

winksaville commented 1 month ago

FYI, I pulled your changes with git pull upstream 6adff635f17326b935640191e1640151f8a72e09 to my repo and then changed the push_str("\n") to push('\n') and works perfectly

wink@3900x 24-05-24T17:26:43.029Z:~/prgs/SurrealDB/surrealist (pp)
$ git diff
diff --git a/src-tauri/src/config.rs b/src-tauri/src/config.rs
index d3bf752..67890ad 100644
--- a/src-tauri/src/config.rs
+++ b/src-tauri/src/config.rs
@@ -17,7 +17,7 @@ fn write_config(config: &str) {
     let config_json_value: serde_json::Value = serde_json::from_str(config).unwrap();
     let mut pretty_config = serde_json::to_string_pretty(&config_json_value).unwrap();

-    pretty_config.push_str("\n");
+    pretty_config.push('\n');

     write_op
         .write_all(pretty_config.as_bytes())
wink@3900x 24-05-24T17:26:49.721Z:~/prgs/SurrealDB/surrealist (pp)
$ 

And then ran pnpm traui:dev, and of course worked:

wink@3900x 24-05-24T17:23:15.209Z:~/.config/SurrealDB/Surrealist
$ cat -n config.json
     1  {}
wink@3900x 24-05-24T17:23:24.399Z:~/.config/SurrealDB/Surrealist
macjuul commented 1 month ago

Feel free to push the change when you're ready so I can merge the PR 🙂

winksaville commented 1 month ago

Feel free to push the change when you're ready so I can merge the PR 🙂

Done

winksaville commented 1 month ago

For a fairly trivial change, although I think helpful, that was somewhat arduous, txs for working with a noob to get it done :)

macjuul commented 1 month ago

Haha no problem, I'm always happy to help