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: add support for color, style, width and alignment to format #769

Merged
merged 11 commits into from
Sep 28, 2023

Conversation

andresovela
Copy link
Contributor

@andresovela andresovela commented Aug 1, 2023

This PR extends the functionality of the --log-format and --host-log-format options implemented in #765 by adding support for color, style, width and alignment options.

The format options are pretty much as described in here:

I can easily imagine assigning static colors, maybe like this "{t:black:bold} [{L:green:dimmed}] Location<{f:yellow:italic}:{l:#5F2A2A:normal}> {s:white:underline}". Note that I A) am using the terms from the colored crate, B) not particularly like that format, it is just what I came up with right now and C) would pick :black:normal as a default.

An example of what the new format could look like is:

{t:>8} [{L:severity:bold}] {f:white:underline}:{l:white:3} {s:werror}

The #5F2A2A variant for colors is not yet supported in this PR - I think the currently implemented options are more than enough.

Something else I still want to implement is support for nested format options or something similar that would allow users to align logs like this:

main.rs:15          This is a log
decoder.rs:345      This is another log

With the current formatting options this is not possible. I think something like {{f}:{l}:20:white:underline} would be a nice syntax to support formatting groups of segments at once. I'll likely wait for this PR to be merged before working on that though.

One open question is how to implement support for styling String log segments, i.e. how do we allow the user to have something like (HOST) [{L}] {s} where the (HOST) part could be blue, dimmed, etc. It's probably not very important and I can't think of many use cases for this.

Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR. I like it and will play around a bit more with it and see how it behaves.

One thing I am wondering is, if we can rely a bit more on the functionality from the colored crate to make the code a bit more simple on our side. E.g. we could use the FromStr implementation from colored::Color to simplify the parsing code. Or we could use colored::Styles instead of our own LogStyle.
I would be against exposing colored types in our public API, but as long as they are only internal I think it makes sense to use them and not hand-roll everything by ourselves.

decoder/src/log/stdout_logger.rs Outdated Show resolved Hide resolved
decoder/src/log/format.rs Outdated Show resolved Hide resolved
decoder/src/log/format.rs Outdated Show resolved Hide resolved
decoder/src/log/format.rs Outdated Show resolved Hide resolved
decoder/src/log/format.rs Outdated Show resolved Hide resolved
decoder/src/log/format.rs Outdated Show resolved Hide resolved
decoder/src/log/format.rs Outdated Show resolved Hide resolved
decoder/src/log/stdout_logger.rs Outdated Show resolved Hide resolved
decoder/src/log/stdout_logger.rs Outdated Show resolved Hide resolved
decoder/src/log/stdout_logger.rs Outdated Show resolved Hide resolved
@andresovela
Copy link
Contributor Author

Hi @Urhengulas, is there any news about this PR?

@Urhengulas
Copy link
Member

Hey, I've been sick and busy with other things recently. Will come back to it next week.

@andresovela
Copy link
Contributor Author

Get well soon :)

Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The happy path works nicely.

I just think the error reporting needs to be slightly improved. I found following cases, which both result in the warning and erroneous output below. I assume it is due to a parsing error.

The first case should be rejected, because it is a mistage. I think abort is fine, since the parsing of the log_format will happen at the start of the program.

The second case should be supported in the long run, but it is fine if we error for now.

  • {t:bold:bold} [{L}] {f}:{l} {s}
  • {t:bold:underline} [{L}] {f}:{l} {s}
$ DEFMT_LOG=info cargo rb levels
    Finished dev [optimized + debuginfo] target(s) in 0.04s
     Running `/home/urhengulas/Documents/github.com/knurling-rs/krate/./probe-run --chip nRF52840_xxAA --log-format '{t:bold:bold} [{L}] {f}:{l} {s}' target/thumbv7em-none-eabihf/debug/levels`
(HOST) WARN  `defmt::timestamp!` implementation was found, but timestamp is not part of the log format; consider adding the timestamp `{t}` argument to the log format
────────────────────────────────────────────────────────────────────────────────



printlnnn
────────────────────────────────────────────────────────────────────────────────
(HOST) INFO  program has used at least 0.23/254.93 KiB (0.1%) of stack space
(HOST) INFO  device halted without error

@Urhengulas
Copy link
Member

Also I just realized that "--host-log-format", "{t} [{L}] {f}:{l} {s}", results in <time> [INFO ] mod.rs:131 device halted without error (note the <time>. But this should not be handled in this PR. In theory it should error, but in the long run we might want to add a host timestamp and then it should work.

@andresovela
Copy link
Contributor Author

Thanks for your feedback :)

I believe I have fixed the issues with the duplicated style specifiers. I also added some tests for that.

Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@Urhengulas Urhengulas added this pull request to the merge queue Sep 28, 2023
Merged via the queue into knurling-rs:main with commit 49b7a96 Sep 28, 2023
14 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.

2 participants