Add `--format` argument to `nickel query` command

[suimong]

Closes #1976

This PR adds the ability for nickel query to export the result to JSON, YAML & TOML, similar to that of nickel export. The output schema looks like:

{
  doc | Nullable String,
  "optional" | Bool,
  not_exported | Bool,
  priority | String,   # variants include: "default", "neutral", "force", and string representation of rational number ("1", "5/2", etc.)
  type | Nullable String,
  contracts | Array String,
  sub_fields | Nullable (Array String),
  value | String | optional,
}

During implementation, I found a few areas that ask for "designing":

• whether to use Nullable T or T | optional for some fields in the output schema
• how to serialize the priority field
• the logic that dictates the presence of the value field

[yannham]

I think it's reasonable. Those options are useful to minimize the markdown output when one is only interested in one particular field, but It's probably less useful for a JSON export, which can be transformed afterwards by another tool (like...Nickel :imp: ?) or the end consumer if needed. I don't remember clap well enough to know if we can encode that constraint directly. I suppose we can (such that the CLI prints an error automatically if you specify both --doc and --format). In any case, as long as it's documented properly in the argument description and validated in one way or another, it's fine by me :+1:

[suimong]

Cool! Will get the PR ready for review soon.

[suimong]

@yannham Thanks for the review! I'm very sorry that I forgot to push one last commit before marking this ready for review.

Additionally, here are a few questions on which I'd like to have your opinion:

• whether to use Nullable T or T | optional for some fields in the output schema
• how to serialize the priority field. Currently everything is serialized to a string, including the Numeral variant. An alternative would be to separate the Numeral variant from Bottom and Top, serializing the Numeral variant to json number.
• the logic that dictates the presence of the value field.

[github-actions[bot]]

Bencher

Report	Thu, September 12, 2024 at 01:55:06 UTC
Project	nickel
Branch	2015/merge
Testbed	ubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

• Latency (latency)

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns)
fibonacci 10	➖ (view plot)	490,370.00
pidigits 100	➖ (view plot)	3,156,400.00
product 30	➖ (view plot)	807,650.00
scalar 10	➖ (view plot)	1,462,600.00
sum 30	➖ (view plot)	784,160.00

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

[yannham]

Hi @suimong, sorry for the long pause - summer break it was!

To answer your previous questions:

whether to use Nullable T or T | optional for some fields in the output schema

I would tend to use T | optional, if it's not too annoying. I feel like this is the usual way in the JSON world/API world rather than putting explicit nulls. The only reason to use null for me is if the data might be used in a statically typed Nickel block, but here it's just for export on the CLI.

how to serialize the priority field. Currently everything is serialized to a string, including the Numeral variant. An alternative would be to separate the Numeral variant from Bottom and Top, serializing the Numeral variant to json number.

This is a good question. I was going to say that a string is a good approximation, and is easy to parse back as an integer if needed, but on the other hand I don't know if lose anything by being more precise and serialize a number as a number. Maybe the schema is less nice to consume for statically typed language (although you can always serialize that as an enum in Rust I guess). I don't have a strong opinion here.

the logic that dictates the presence of the value field.

Hmm. I'm tempted to revisit the original logic and to just always print the value (capped so that we don't print 20k lines of config when querying a top-level attribute). But this can be done in a second step; for now, if it's not too annoying, I would keep the same logic as the original query printer, if possible. We can then revisit both at once later.

[suimong]

Hi @yannham hope you had a great summer break and thanks for the review!

~~I rebased the PR onto the latest master, and it should be ready to merge.~~ ~~There's still some work to be done. I'll ping you again when it's ready.~~ It's done.

I edited the first comment to better reflect the changes made in this PR.

I would tend to use T | optional, if it's not too annoying. I feel like this is the usual way in the JSON world/API world rather than putting explicit nulls. The only reason to use null for me is if the data might be used in a statically typed Nickel block, but here it's just for export on the CLI.

Got it. ~~This part still needs a bit more work. I'll ping you when it's ready.~~

This is a good question. I was going to say that a string is a good approximation, and is easy to parse back as an integer if needed, but on the other hand I don't know if lose anything by being more precise and serialize a number as a number. Maybe the schema is less nice to consume for statically typed language (although you can always serialize that as an enum in Rust I guess). I don't have a strong opinion here.

I'm going to keep the original string representation which as you said is a fairly good approximation. There's something interesting here (I should've mentioned when I asked the question), as I found out that the actual type of the priority "number" is actually Rational, which means if a user specifies a priority of 2.336, it will be serialized to "292/125"😄 While this will arguably make parsing it into number somewhat harder, but in all practicality, since users will need to write custom logic to parse the "priority" field anyway, this should not cause them too much extra work.

On a side note, I realized that making the priority a rational number is a pretty neat feature, as you'll never run into a situation where you cannot "insert" a layer of config in between two layers. With integer, while in theory user could space the priority number of two adjacent layers far apart, like priority 10, priority 20 etc., but in practice a pretty common case is that one layer does not define priority (priority 0), and some user defines an override with priority 1 (because that's just intuition), and now you can not sneak in anything in between. But with rational numbers, we can just set priority to the average of the two number.

[suimong]

I'm not sure why there's a merge conflict...I thought I solved all of them locally.

[jneem]

It seems to be rebased on an older master. For example, I see 165eedd on master, but not in this branch

[suimong]

@jneem thanks for taking a look. Silly me. @yannham Now that all CI lights are green, I think it's ready. sorry about the noise.

[suimong]

Thanks again for all the guidance! Alas, there seems to be a bug wrt the query result (IMHO it is unrelated to this PR):

It apperas that when the field's value is a record containing a subfield whose value is a string, nickel mistakenly identifies the subfield as a closure.

[yannham]

It's strange that a string literal is closurized here, but it's a technical detail that I think is indeed unrelated to this PR - we might be missing some closure substitution somewhere. However it makes something else apparent: given how serde works, value is recursively serialized, which is an issue (typically in your case, it doesn't know how to serialize a closure - and there are many expressions that aren't serializable to JSON, such as foo + 1, if true then 1 else 2 or fun x => x + 1, and they can all be the value of a field). So I think what we want is for value to be an Option<String> which is the original value pretty-printed, instead of deeply serializing it (it will print the ugly closure as well, but it's a question that we can handle separately).

[suimong]

Ah that's what you meant by "print" & "capped" in an earlier comment. It's fixed and I added a note in the CLI's help message regarding the value field.

[GuillaumeDesforges]

Thank you very much for this improvement :heart: