preserve space by letting to nuon only add quotes when necessary

[merelymyself]

Description

Related: #6376 (comments), #6283

Quotes are now only used when absolutely needed, i.e. there is a space or comma in the name.

Tests

Make sure you've done the following:

• Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
• Try to think about corner cases and various ways how your changes could break. Cover them with tests.
• If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

• cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• cargo test --workspace --features=extra to check that all the tests pass

[sholderbach]

I like the intention. But I am not sure how we want to develop the .nuon format. If it should follow JSON as a superset or try to be as close to regular handwritten nu code.
If we want to be as close to an extended JSON, string keys would have to be escaped as explicit strings anyways.

[kubouch]

I'd vote for the "close to the handwritten Nushell" option. The important part is that any JSON is a valid NUON, but the other way is not so important. NUON already supports a lot of things that are not close to JSON at all, like the table syntax. We can make an advantage of this to make NUON more user-friendly and space-efficient.

Maybe @fdncred has an opinion as well?

[fdncred]

My goal for pushing for this PR was to make nuon as small as possible, so removing extraneous quotes helps that. The reason why I want it to be small is because I think that was one of the founding principles for nuon. I remember JT comparing size against many other serialization formats and nuon was pretty much beating them all. So, I'd vote for nuon to be as close to handwritten nushell as possible. I'd also like nuon to support all our syntax including things like blocks so we can serialize/deserialize 100% of nu-lang.

[kubouch]

Looking at the list of the blacklisted characters, this PR would really use some proper smoke test. Like testing all sorts of weird strings as column names and making sure we can to nuon | from nuon correctly. Whitespace characters like tab or newline should be blacklisted as well.

[sholderbach]

Maybe fuzzing with proptest for a succesful roundtrip?

[kubouch]

Yeah, we need fuzzing but that's out of scope of this PR. Fuzzing probably should be in a separate place and in a separate CI step to keep the rest of the Nushell tests deterministic. It would be quite annoying if you had a random fuzzing failure on your PR that has nothing to do with your changes.

Here, I would just think of all sorts of weird cursed strings and make sure we can do the roundtrip.

[sholderbach]

True, we shouldn't fuzz on the PR workflow. But it seems that the proptest crate supports some features to persist previous failure cases https://docs.rs/proptest/0.7.2/proptest/#failure-persistence (Don't know if one can run those examples without executing proptest or if we have to manually extract unit tests from the failure log)

[fdncred]

ok, it seems like we have consensus that this PR is fine and we need to use proptest (or something else) on some future PR to fuzz test a bit.

[kubouch]

I'd still put more tests to make sure names with all weird characters are quoted correctly (or disallowed as column names, either way).

I think currently missing are newlines, tabs or pipes. There might be more.

[fdncred]

oops, sorry. i thought we were ready. @merelymyself can you add a few more tests like kubouch suggests please?