Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

colorize defmt::assert_eq output #110

Merged
merged 3 commits into from
Nov 24, 2020
Merged

colorize defmt::assert_eq output #110

merged 3 commits into from
Nov 24, 2020

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 23, 2020

this implementation parses a formatted log message and checks if the output corresponds to the defmt::assert_eq! macro. If that's the case then it applies color using the same approach as the pretty-assertions crate.

a better implementation would be to tag the log messages from the defmt::assert_eq and apply the coloring based on the tag, instead of re-parsing the formatted log message. however, that doesn't seem possible to implement in the v0.1.x line because:

  • if we add a new variant to decoder::Tag that would be a breaking change (UB?) because the enum is not repr(C)
  • if we try to apply to formatting when the SymbolTag::Custom tag is present then old versiosn of probe-run will not print the assert_eq message (as they ignore custom tags)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jonas-schievink
Copy link
Contributor

if we add a new variant to decoder::Tag that would be a breaking change (UB?) because the enum is not repr(C)

Did you mean #[non_exhaustive] instead of #[repr(C)] here? (I agree with the conclusion anyways)

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Cool!

src/logger.rs Outdated
}
Difference::Add(_) => continue,
Difference::Rem(s) => {
write!(buf, "{}", Colour::Red.on(Colour::Fixed(52)).bold().paint(s)).ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the 52 to a constant?

src/logger.rs Outdated
write!(
buf,
"{}",
Colour::Green.on(Colour::Fixed(22)).bold().paint(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

(same here)

@japaric
Copy link
Member Author

japaric commented Nov 24, 2020

Did you mean #[non_exhaustive] instead of #[repr(C)] here? (I agree with the conclusion anyways)

yes. (in my mind, repr(C) (+ lots of care) is needed if you serialize C-like enums that you'd like to extend without breaking things. we are not binary (de)serializing the tags so non_exhaustive should suffice -- I was under the original impression that we did serialize them but was wrong)

@japaric japaric merged commit be2448e into main Nov 24, 2020
@japaric japaric deleted the color-diff branch November 24, 2020 13:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants