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

feat: add report formatter trait for custom reporter output #158

Merged
merged 9 commits into from
Nov 30, 2023

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Nov 30, 2023

Adds a ReportFormatter trait and a Reporter::report_with_formatter method allowing customization of formatting the report without reimplementing reporter internals. Currently, we use format_external and format_terms but we can refine formatting in the future as needed e.g. all the explain methods in the reporter.

Adds a DefaultStringReportFormatter which implements the existing format for the DefaultStringReporter.

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

@@ -17,8 +17,15 @@ pub trait Reporter<P: Package, VS: VersionSet> {
type Output;
Copy link
Member

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?

Copy link
Member Author

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;
Copy link
Member

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?

Copy link
Member Author

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.

src/report.rs Outdated Show resolved Hide resolved
@mpizenberg
Copy link
Member

I like it a lot too!

@zanieb zanieb mentioned this pull request Nov 30, 2023
Copy link
Member

@Eh2406 Eh2406 left a 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?

@mpizenberg
Copy link
Member

Nothing to add, this is great!

@zanieb zanieb changed the title Add report formatter concept for customization of report output feat: add report formatter concept for customization of report output Nov 30, 2023
@zanieb zanieb added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@zanieb zanieb changed the title feat: add report formatter concept for customization of report output feat: add report formatter trait for custom reporter output Nov 30, 2023
@zanieb zanieb added this pull request to the merge queue Nov 30, 2023
Merged via the queue into dev with commit 4d78a64 Nov 30, 2023
5 checks passed
@zanieb zanieb deleted the zb/report-formatter-upstream branch November 30, 2023 23:49
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.

4 participants