Skip to content

Conversation

@Nemo157
Copy link
Contributor

@Nemo157 Nemo157 commented Nov 3, 2025

I ended up doing a bit more refactoring of the pretty-printer to get it both testable and a format that I think is readable enough to merge. With the test you can step through the commits and see how the format changes with each.

I've targeted this for @arctic-alpaca's branch and propose we merge into that first, then merge it together as a whole.

@claude
Copy link

claude bot commented Nov 3, 2025

Change Summary

This PR refactors the ConsolePrettyExporter to improve formatting and testability. Key changes include converting the unit struct to a tuple struct wrapping an Output enum, reformatting log output with millisecond timestamps and padded columns, and adding comprehensive test coverage with an in-memory buffer variant.

Issues Found

I've identified 3 issues:

  1. 🟡 Documentation inaccuracy (line 6): Doc comment states output goes to stdout, but implementation writes to stderr
  2. 🟡 Error handling (line 205): Empty panic!() lacks descriptive message
  3. 🟢 Minor formatting (line 82): Unnecessary trailing comma in writeln! macro

The refactoring successfully achieves its goals of improved formatting and testability. The test coverage is comprehensive with both boot-time and Unix timestamp scenarios.

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

❌ Patch coverage is 96.93878% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
veecle-telemetry/src/collector/pretty_exporter.rs 96.90% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Nemo157 Nemo157 force-pushed the wim/push-kznypllkvuyt branch from f127b98 to e068544 Compare November 3, 2025 15:56
@Nemo157
Copy link
Contributor Author

Nemo157 commented Nov 3, 2025

/claude-review

@claude
Copy link

claude bot commented Nov 3, 2025

Change Summary

This PR refactors the ConsolePrettyExporter to improve formatting consistency and testability. The changes convert the unit struct to a tuple struct with a DEFAULT constant for static usage, extract formatting logic into a testable format_message helper function, improve log output formatting with millisecond timestamps and padded severity/time columns, fix string value display to use Debug formatting (adding quotes), and add comprehensive test coverage demonstrating both boot-time and Unix timestamp scenarios.

All previously identified issues have been addressed in the latest commits.

✅ Review complete - no new issues found

use std::vec::Vec;

#[test]
fn smoke_test() {

Choose a reason for hiding this comment

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

I personally prefer naming this test by what it tests, but is not a blocker. Feel free to resolve.

* Don't show any trailing text if there are no attributes
* Use [] instead of "" to delimit the attributes
* Remove leading `, ` before the first attribute
* Debug print strings to tell where they end

Signed-off-by: Wim Looman <[email protected]>
Co-authored-by: arctic-alpaca <[email protected]>
@Nemo157 Nemo157 force-pushed the wim/push-kznypllkvuyt branch from ff0c090 to bf9f4e6 Compare November 13, 2025 10:12
@arctic-alpaca arctic-alpaca merged commit 8fa7263 into julian/pretty-printer Nov 14, 2025
22 checks passed
@arctic-alpaca arctic-alpaca deleted the wim/push-kznypllkvuyt branch November 14, 2025 09:07
arctic-alpaca added a commit that referenced this pull request Nov 14, 2025
* Refactor of the pretty-printer to make it testable.
* Improve formatted output.
* Print to stderr instead of stdout.

---------

Signed-off-by: Wim Looman <[email protected]>
Co-authored-by: arctic-alpaca <[email protected]>
arctic-alpaca added a commit that referenced this pull request Nov 14, 2025
* Refactor of the pretty-printer to make it testable.
* Improve formatted output.
* Print to stderr instead of stdout.

---------

Signed-off-by: Wim Looman <[email protected]>
Co-authored-by: arctic-alpaca <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Nov 19, 2025
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.

5 participants