Skip to content
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

decoder: formatting improvements #783

Merged
merged 8 commits into from
Jan 10, 2024

Conversation

andresovela
Copy link
Contributor

@andresovela andresovela commented Oct 3, 2023

Besides refactoring the log formatting code, this PR introduces a few new features:

{c} metadata specifier

Now it's possible to add {c} to the log format in order to print the crate name. I'm currently parsing this out of the module path, there may be better ways to do this.

More detailed {f} metadata specifiers

I found the {f} specifier to be not very useful in most cases, mainly because a lot of files are called mod.rs. Now it's possible to increase how detailed to print the path to the file by adding {ff}, {fff}, etc... to the log format. With {ff} the Formatter would print format/mod.rs instead of just mod.rs, which I think is much more useful.

Zero-padding support

I added the zero-padding functionality from #780 to this PR. Now it's possible to zero-pad metadata specifiers, such as the timestamp or the line number.

@Urhengulas
Copy link
Member

I am actually a bit unsure how to go about it. Yes, the default format logic is duplicated in knurling-rs/probe-run#425, but it is actually a bit different than in probe-run and also different than in this PR. Yes they check if there is a timestamp or not, but they also have a --no-location flag. So the code on that side would not necessarily get more simple by this PR. Also note that they are not using log and therefore not defmt_decoder::log::init_logger.

@andresovela
Copy link
Contributor Author

Yes they check if there is a timestamp or not, but they also have a --no-location flag.

I'd then propose to add a no-location flag to DefmtLoggerConfig. I don't see why not, it seems like it would be a fairly common configuration option.

Also note that they are not using log and therefore not defmt_decoder::log::init_logger

I think all of this logic would end up in the Formatter introduced in #781, so I don't see this as an issue.

@Urhengulas
Copy link
Member

Makes sense

@andresovela andresovela changed the title decoder: add default formats and timestamp warning logic decoder: refactor formatting stuff Oct 4, 2023
@andresovela
Copy link
Contributor Author

I did a relatively big refactor of all the formatting stuff.

@Urhengulas
Copy link
Member

I did a relatively big refactor of all the formatting stuff.

Cool, thank you! I will hand over the project to a colleague this week. He will come back to you.

@andresovela andresovela changed the title decoder: refactor formatting stuff decoder: formatting improvements Oct 15, 2023
@andresovela
Copy link
Contributor Author

Hi @Urhengulas, do you know if your colleague will have time for some reviews soon?

@Urhengulas
Copy link
Member

Hi @Urhengulas, do you know if your colleague will have time for some reviews soon?

@BriocheBerlin @amanjeev

@jonathanpallant
Copy link
Contributor

I'll take a look now.

@jonathanpallant
Copy link
Contributor

Looks good to me. It'll need a rebase to fix the CHANGELOG, and perhaps we could squash the first three commits, just for neatness.

@andresovela
Copy link
Contributor Author

Rebased now :)

@andresovela
Copy link
Contributor Author

Ping @jonathanpallant

decoder/src/log/format/mod.rs Outdated Show resolved Hide resolved
@Urhengulas Urhengulas merged commit d59c36f into knurling-rs:main Jan 10, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants