Introduce new public Reporter trait that is used from CLI types to report file status
#309
Conversation
The `Querier` is a reusable type that should not assume it is used in a console output where printing things is okay. This change ensures all output is reported to the reporter only.
Performance SummaryComparing base 134b7bc with head 0fcb846 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
| use crate::CancellationFlag; | ||
|
|
||
| use super::util::reporter::ConsoleReporter; | ||
| use super::util::reporter::Level; | ||
| use super::util::CLIFileReporter; | ||
|
|
There was a problem hiding this comment.
nit: I would suggest not having both crate references and super references — can you change these to crate::util::[etc]? (Unless there's a reason for the super references that I missed)
There was a problem hiding this comment.
No, this is a good suggestion. Perhaps VSCode does this automatically? At least I don't remember doing this inpurpose.
| @@ -373,7 +398,8 @@ impl TestArgs { | |||
| filter: &dyn Filter, | |||
| success: bool, | |||
| cancellation_flag: &dyn CancellationFlag, | |||
| ) -> anyhow::Result<()> { | |||
| ) -> anyhow::Result<Vec<String>> { | |||
| let mut outputs = Vec::with_capacity(3); | |||
There was a problem hiding this comment.
It looks like outputs is only ever used by joining the string elements into a larger string delimited by newlines. Is it worth building up a single already-joined String here? String implements std::fmt::Write, so all of your outputs.push(format!(...)) calls would turn into write!(&mut output, ...).
That said, this suggestion might be premature optimization, since this code path is only called when running our test suite.
| /// | ||
| /// For each file, either | ||
| /// - [`skipped`] is called once, or | ||
| /// - [`started`] and one of [`succeeded`], [`failed`], or [`canceled`] are called. |
There was a problem hiding this comment.
"and one" ⇒ "and then one" to describe the required ordering of calls
There was a problem hiding this comment.
Yep, that's better!
|
Does this need a changelog entry? |
Performance SummaryComparing base 134b7bc with head 0fcb846 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
Fixes #307 by introducing a public
Reportertrait that users can implement for custom logging.Approach
File status is reported through a
Reportertrait. To keep the burden on implementors limited, we require a predicatable way of calling reporters:skipped, orstartedand on of (succeeded,failed, orcanceled).Implementation
A
ConsoleReportertype is provided that prints to the console and allows control over what is logged.The implementations if
IndexerandQueriertake some short-cuts, and violate this protocol. That is solved by theCLIFileLoggerutility class, that retains that part of the logic from the oldLoggerimplementation.The
Querierhas also been updated to use the reporter for all its output. Previously, it was still printing directly to the console in some cases.