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(search): add matches number to summary #3747

Merged
merged 8 commits into from
Sep 2, 2024

Conversation

BackupMiles
Copy link
Contributor

Summary

This PR aims to provide a summary of the search command, citing how many matches were found during the operation.

Test Plan

No tests have been provided.

@github-actions github-actions bot added the A-CLI Area: CLI label Aug 31, 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 work! Just two minor suggestions. It looks like there's still a lint issue and snapshots need to be updated.

@@ -27,6 +27,8 @@ pub(crate) enum FileStatus {
Unchanged,
/// While handling the file, something happened
Message(Message),
/// A match was found while searching a file
Search(Message),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Match or SearchResult is a better name? It doesn't seem to be used generically for messages from the search command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SearchResult sounds better, thank you 🙏

@@ -691,6 +700,11 @@ fn handle_file(ctx: &TraversalOptions, path: &BiomePath) {
Ok(Ok(FileStatus::Unchanged)) => {
ctx.increment_unchanged();
}
Ok(Ok(FileStatus::Search(msg))) => {
ctx.increment_changed(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

What purpose is there for bumping changed? It feels semantically off, and I don't see why it's necessary...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I meant to increment the unchanged files, aka when a match is found in a file it will still report that file as "searched"

@arendjr arendjr added this to the Biome 1.9 milestone Sep 1, 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.

👍 Just a lint still.

@BackupMiles BackupMiles changed the title feature: add matches number feat(search): add matches number to summary Sep 2, 2024
@arendjr arendjr merged commit ac0408f into biomejs:main Sep 2, 2024
10 of 11 checks passed
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.

2 participants