-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add --format
argument to nickel query
command
#2015
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yannham do you think it's ok that in this PR, the --format
flag ignores all customzation flags (--doc
, --default
etc.) and just exports all metadata fields and value? I tried to make them compatible but after some experimentation I'm inclined to keep the two features independent. The main reason is that by default --format
exports all metadata fields which I think is desirable, but a few of them does not have a corresponding flag (namely optional
, not_exported
, priority
), so customization for json export would be half baked and inconsistent with the current markdown output. That is, if we leave the markdown rendering code untouched.
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 👿 ?) or the end consumer if needed. I don't remember |
Cool! Will get the PR ready for review soon. |
cli/src/query.rs
Outdated
let query_res = program.query().map(|field| QueryResult(field)); | ||
if let Ok(res) = query_res { | ||
self.export(res, format).report_with_program(program)? | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: instead of duplicationg "No metadata found for this field"
, could we define found
outside of the if let Some(format) = self.format {...} else {...}
and just set found to true
in theif
(or false
in the else, depending on the initial value of found
), and move the if !found
below also after the whole if-then-else?
core/src/serialize.rs
Outdated
#[default] | ||
Json, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we shouldn't rather have an additional Markdown
variant, which is the default (and should be the documented default in the CLI as well). I think it's good practice that the normal behavior (without --format
) can be recovered in a more precise way with an argument - typically, nickel export
is the same as nickel export --format json
, etc. - so that the default behavior is just that, a default value.
On the other hand, it makes the logic of when we should allow --doc
, etc. more complex. So I'm not entirely sure. Maybe we can start as it is now, but then I would remove the Default
impl of ExportFormatCommon
, because it doesn't really have a default value.
core/src/term/mod.rs
Outdated
impl Serialize for MergePriority { | ||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
where | ||
S: Serializer, | ||
{ | ||
match self { | ||
Self::Bottom => serializer.serialize_unit_variant("priority", 0, "default"), | ||
Self::Neutral => serializer.serialize_unit_variant("priority", 3, "neutral"), | ||
Self::Numeral(n) => serializer.serialize_newtype_variant("priority", 2, "numeral", &n), | ||
Self::Top => serializer.serialize_unit_variant("priority", 3, "force"), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty mechanic - can't serde
derive it automatically with #[derive(Serialiaze)]
on MergePriority
?
core/src/serialize.rs
Outdated
@@ -47,6 +46,25 @@ impl fmt::Display for ExportFormat { | |||
} | |||
} | |||
|
|||
/// Available common export formats. | |||
#[derive(Copy, Clone, Eq, PartialEq, Debug, Default, clap::ValueEnum)] | |||
pub enum ExportFormatCommon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExportFormatCommon
isn't very telling, IMHO (how it is different than ExportFormat
or other formats described elsewhere in the codebase?). If there's no good general way to describe it, maybe something more descriptive such as MetadataExportFormat
is less confusing?
@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:
|
|
Report | Fri, August 9, 2024 at 06:21:04 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) | 489,740.00 |
pidigits 100 | ➖ (view plot) | 3,193,800.00 |
product 30 | ➖ (view plot) | 787,510.00 |
scalar 10 | ➖ (view plot) | 1,471,300.00 |
sum 30 | ➖ (view plot) | 786,270.00 |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
Closes #1976
This PR adds the ability for
nickel query
to export the result to JSON, YAML & TOML, similar to that ofnickel export
. The output schema looks like:During implementation, I found a few areas that ask for "designing":
Nullable T
orT | optional
for some fields in the output schema