Live check output formats#1157
Conversation
…live-check-output-formats
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1157 +/- ##
=====================================
Coverage 80.1% 80.1%
=====================================
Files 108 109 +1
Lines 8440 8526 +86
=====================================
+ Hits 6761 6831 +70
- Misses 1679 1695 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is there anything in this PR you'd change based on |
I'm hoping not. I think I've made it general purpose. I think resolve or another one already has some code for serde output directly so I wanted to do that refactoring in a follow up to avoid huge PRs. |
jsuereth
left a comment
There was a problem hiding this comment.
Great addition!!!
mostly just nits or thoughts on improvement.
Looking forward to using this one.
| fn serialize<T: Serialize>(&self, data: &T) -> Result<String, Error> { | ||
| match self { | ||
| BuiltinFormat::Json => { | ||
| serde_json::to_string_pretty(data).map_err(|e| Error::SerializationError { |
There was a problem hiding this comment.
Nit: Should we have a json_pretty and json as separate options?
There was a problem hiding this comment.
I think jsonl is the json_ugly. The way I've written jsonl handling leaves it to the caller to determine where they want the newlines in the output. You could just have one for the entire output.
|
|
||
| /// Output processor - handles output generation with builtin formats or templates | ||
| #[allow(clippy::large_enum_variant)] | ||
| pub enum OutputProcessor { |
There was a problem hiding this comment.
Nit:
This may be better as a trait. If we need a container to wrap it, we could do something like:
struct OutputProcessorOpt(dyn OutputProcessor);
impl <T: OutputProcessor> From<T> : OutputProcessorImpl {
fn from(p: T) -> OutputProcessorImpl { OutputProcessorImpl(p) }
}
There was a problem hiding this comment.
I'll look at a refactoring... the clippy thing is not a big deal IMO since you make one of these not thousands.
| } | ||
|
|
||
| /// Open/create the output file if not already open. | ||
| fn open(&mut self) -> Result<(), Error> { |
There was a problem hiding this comment.
These kinds of "implementation deferred" functions are why I think it may be easier to use a trait instead of enum here.
It's a bit hard to follow the stateful behavior across the enum when all the methods are in the same impl.
There was a problem hiding this comment.
As above... I'll look at that refactoring.
| } | ||
|
|
||
| /// Create/truncate a file and return the handle | ||
| fn create_file(path: &std::path::Path, filename: &str) -> Result<File, Error> { |
There was a problem hiding this comment.
Nit - this could be a free-standing helper function.
There was a problem hiding this comment.
This and open are supposed to be the hidden internals. I guess this might be useful elsewhere??
There was a problem hiding this comment.
I'd make it free-standing and hidden to the file, and if we want to publicize it later we can easily do so without moving it around.
| }) | ||
| } | ||
| OutputDirective::Stderr => { | ||
| unreachable!("OutputProcessor does not support Stderr directive") |
There was a problem hiding this comment.
Why's that? Should we support stderr?
There was a problem hiding this comment.
I will revisit this when I integrate OutputProcessor with the other weaver commands. StdErr should be reserved for the logger output so we don't mix artifacts with runtime information. I think this may change for diagnostic output, but I need to get to that.
There was a problem hiding this comment.
Yeah - I'm relying on diagnostic output for the weaver-package tests, hence why I'm really curious how we do this going forward :)
|
|
||
| /// Returns the builtin format if this is a builtin processor | ||
| #[must_use] | ||
| pub fn builtin_format(&self) -> Option<BuiltinFormat> { |
There was a problem hiding this comment.
NIt: Is this needed publicly? Anyway we could hide this?
src/registry/live_check.rs
Outdated
| samples: Vec<Sample>, | ||
| stats: LiveCheckStatistics, | ||
| ) -> Result<(), weaver_forge::error::Error> { | ||
| // Special JSONL handling: one line per sample, stats at end |
There was a problem hiding this comment.
Could we create something like - "needs_multiple_events()" or "is_streaming()" on the OutputProcessor instead?
If we were to use jsonl other places or if we add other new built-in formats, It'd be nice if we didn't need to alter this logic.
There was a problem hiding this comment.
I went back and forth on this a few times and landed on this API. This handling is specifically for jsonl in live-check's report mode. Most things, I think, will have a single object (albeit large) output. The API here allows you to call generate multiple times and it will append to the file. The first call to generate opens the file.
Again, if this is a bad fit for the other commands I'll refactor.
There was a problem hiding this comment.
Oh, wait, I think you're talking about the call below what you've highlighted. Instead finding out if it's JSONL find out if the format requires multiple calls to generate. Got it.
src/registry/live_check.rs
Outdated
| LiveCheckStatistics::Cumulative(_) => output.generate(&stats), | ||
| LiveCheckStatistics::Disabled(_) => Ok(()), | ||
| } | ||
| } else if matches!(output, OutputProcessor::Mute) { |
There was a problem hiding this comment.
Nit: If Mute processor does nothing, why not just call it?
| OutputDirective::Stdout | ||
| }; | ||
|
|
||
| let mut output = OutputProcessor::new( |
There was a problem hiding this comment.
We need to fix this somehow.
|
@jsuereth - Instead I improved the public API by wrapping the enum in a public struct with private internals. I think I cleaned up all your other nits. |
jsuereth
left a comment
There was a problem hiding this comment.
Nice cleanups!! This looks really good now
Feature (#1132) - Live-check: builtin output rendering for json, jsonl and yaml.
Added an
OutputProcessorto weaver_forge which handles json, jsonl, yaml for builtin formats, templates or mute (no output).The end goal is to unify all weaver output in a standard way. This first PR tackles this only for Live-check only.
A follow-up PR will fit this for other commands:
diffandresolve.