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(cli): generic reporter APIs #853

Merged
merged 13 commits into from
Apr 6, 2024
Merged

feat(cli): generic reporter APIs #853

merged 13 commits into from
Apr 6, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Nov 23, 2023

Summary

This PR implements a new reporter API.

This API uses traits, and it's heavily inspired by the diagnostics advices traits:

  • We have a Reporter trait. Usually the type that implements Reporter is the one that will hold the data to read from.
  • We have a ReporterVisitor. Usually, the type that will implement ReporterVisitor is the one that will hold a buffer where information is written.

I also added an example that should show how to use the reporter.

Having such generic APIs should allow us to create new reporters easily, based on Biome's needs.

I had to do some refactors around the traverse function and the data returned by it.

Test Plan

There are a lot of changes, mainly because now the logs are written at different times instead of once, and our BufferConsole pushes messages inside a vector.

Copy link

netlify bot commented Apr 4, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 12c44c7
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/6610120f89bcf100084f52ef
😎 Deploy Preview https://deploy-preview-853--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (🟢 up 1 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@ematipico ematipico marked this pull request as ready for review April 4, 2024 14:47
@ematipico ematipico requested review from a team April 4, 2024 14:48
@ematipico ematipico changed the title feat: reporter feat(cli): generic reporter APIs Apr 4, 2024
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Nice one! Glad I'm not the only one making big PRs 🤣

I would like it if we could keep the output format a bit more condensed though, even if it requires some amount of "opinionatedness" in the reporter.

}

/// When using this trait, the type that implements this trait is the one that will **write** the data, ideally inside a buffer
pub trait ReporterVisitor {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking a little bit about what would be the best way to design output for the biome search command. Would it report search results through "info" diagnostics? Would it report a a summary? Would it use the reporter infrastructure at all?

We don't need answers to those questions for this PR, but they might be good to keep in the back of our minds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, unfortunately, we don't have a design yet; I suppose we will have to make the APIs better as we implement new features, such as JSON output, writing things in files, and other formats.

/// A type that holds the result of the traversal
#[derive(Debug, Default)]
pub struct TraversalSummary {
changed: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe track processed instead of unchanged? I'm thinking processed = changed + unchanged, so it would change the method of tracking slightly, but I feel changed/unchanged feels a little bit like a mismatch for use cases that aren't intended to change files in the first place, such as biome ci or the upcoming biome search.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, although not in this PR because It seems out of scope

crates/biome_cli/src/reporter/mod.rs Outdated Show resolved Hide resolved
}
} else {
Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cleanup! I like that these concerns got nicely separated in the Reporter trait.

Copy link
Member

Choose a reason for hiding this comment

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

Same there! Looks cleaner 🤩.

@ematipico
Copy link
Member Author

ematipico commented Apr 5, 2024

I would like it if we could keep the output format a bit more condensed though, even if it requires some amount of "opinionatedness" in the reporter.

What do you think if all the methods report_total, report_errors, etc., would return an impl Display instead, and then all these displays get combined inside the buffer inside report_summary?

I'll have to create a reporter::fmt::Display, but I believe we should be able to craft a good API

crates/biome_cli/src/reporter/mod.rs Outdated Show resolved Hide resolved
crates/biome_cli/src/reporter/mod.rs Show resolved Hide resolved
crates/biome_cli/src/reporter/mod.rs Outdated Show resolved Hide resolved
@ematipico
Copy link
Member Author

I addressed all the comments

@ematipico
Copy link
Member Author

FYI I already found some flaws in the initial APIs, so the next PR will further change them to accommodate the JSON reporter

@ematipico ematipico merged commit 719ff7d into main Apr 6, 2024
16 checks passed
@ematipico ematipico deleted the feat/reporter branch April 6, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants