Add pub struct Formatter to defmt_decoder::log.
#781
Conversation
decoder/src/log/stdout_logger.rs
Outdated
| write!(sink, "{s}")?; | ||
| } | ||
| writeln!(sink) | ||
| } | ||
|
|
||
| pub(super) fn format_frame(&self) -> Result<String, std::fmt::Error> { |
There was a problem hiding this comment.
I think it makes sense to move all of the formatting logic to Formatter and pass a Formatter instead to Printer::new(). What do you think?
There was a problem hiding this comment.
I was thinking about that. But the Printer holds a reference to the record aka. the log frame. So you basically have one Printer per log frame, which makes sense because it uses this reference in almost all methods.
The Formatter should not keep a reference to the record, but just receive it for the method call.
There was a problem hiding this comment.
The name Printer does not really make sense anymore. I think SingleRecordFormatter would be the most correct, but maybe a bit bulky. But I'd like to pull the naming discussion out of this PR.
There was a problem hiding this comment.
As I have written as a TODO in code, I think the StdoutLogger should use two Formatters instead of just storing two log_formats. So the hierachy would be:
graph TD
StdoutLogger --> Formatter
Formatter --> Printer
But I'd also like to do that in a separate PR.
There was a problem hiding this comment.
The name Printer does not really make sense anymore. I think SingleRecordFormatter would be the most correct, but maybe a bit bulky. But I'd like to pull the naming discussion out of this PR.
I disagree, the purpose of Printer after this PR should not be to format, but to print. The only reason why SingleRecordFormatter would make sense after this PR, is because you added format_frame to Printer. I think format_frame should be the responsibility of Formatter.
The Formatter should not keep a reference to the record, but just receive it for the method call.
I agree, then in print_frame the Printer would just need to call
write!(sink, "{}", self.formatter.format_frame(&self.record)):
I think the StdoutLogger should use two Formatters instead of just storing two log_formats.
Yeah, I think that makes sense.
|
I am a bit short on time and will therefore merge this PR as is. I linked the discussion from the code and will refactor it at a later point. Otherwise I am open for a refactoring PR. |
The purpose of
Formatteris to format defmt frames according to the log format. Specifically I will be using this to add custom log format support toprobe-rs.@andresovela Would you have time for a review?