-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat: add report formatter trait for custom reporter output #158
Conversation
@@ -17,8 +17,15 @@ pub trait Reporter<P: Package, VS: VersionSet> { | |||
type Output; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this trait pulls its weight, I think users would be just as well served by freestanding functions. Do you want to do that in this PR or leave it for follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this trait is a little weird unless users are expected to be passing around generic reporters. It makes a bit more sense now that there's another method, but I agree it seems sensible to remove. I'd prefer to track this in a new issue.
/// Trait for formatting outputs in the reporter. | ||
pub trait ReportFormatter<P: Package, VS: VersionSet> { | ||
/// Output type of the report. | ||
type Output; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code only supports Output = String
, eventually I prefer these functions return impl Display
or take a &mut Write
. Do you think Output
pulls its weight? We could have the methods return String
for now and fix it in a follow-up, or have the methods take &mut Write
and return the bubbled up error. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to mirror the Reporter
trait for now. We can tackle the rest in a subsequent refactor? I think that design deserves some dedicated consideration.
I like it a lot too! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge step forward! There are more fixes to come, so let's merge this as is. Unless @mpizenberg have anything he wants done in this PR?
Nothing to add, this is great! |
Adds a
ReportFormatter
trait and aReporter::report_with_formatter
method allowing customization of formatting the report without reimplementing reporter internals. Currently, we useformat_external
andformat_terms
but we can refine formatting in the future as needed e.g. all theexplain
methods in the reporter.Adds a
DefaultStringReportFormatter
which implements the existing format for theDefaultStringReporter
.You can see how much easier this is to use in the example diff at 725ec87
Some existing discussion at astral-sh#10
Addresses some of the concerns in #150