feat(search-output-formatter): initialize search output formatter #3258

BackupMiles · 2024-06-22T15:55:54Z

Summary

This PR aims to provide the infrastructure for the future search command output.

Added the capability to highlight a given TextRange inside a file
Added a dummy data provider to generate the text matches temporarily

Aims to resolve #2462

Test Plan

Since this PR only introduces an infrastructure for a command output, I have not provided any tests yet -- I would be glad to add them if deemed necessary.
Running cargo test still yields a positive result.

arendjr

Nice work!

arendjr · 2024-06-22T21:31:16Z

crates/biome_diagnostics/src/display/frame.rs

+    let source = SourceFile::new(source_code);
+
+    let start = source.location(span.start())?;
+    let end = source.location(span.end())?;


Ah, this makes me realize we need to calculate line numbers for every match, which can potentially be a bit expensive. No big deal either way, but a good reminder that I may want to see if the Grit engine can already report the line numbers for us. At least it tracks them for some purposes already (though not everywhere), so I may be able to pull those out.

Would you like me to add a TODO to keep track of this possibility?

Would be appreciated!

arendjr · 2024-06-22T21:35:52Z

crates/biome_diagnostics/src/display/frame.rs

+        } else if is_last_line {
+            let start_index = current_text
+                .text_len()
+                .checked_sub(current_text.trim_start().text_len())


I think I’d just use saturated_sub() for the extra safety. It shouldn’t happen either way.

Sure! Quick note, with that function being available on u32 I would need to add a bunch of into()-s, and later transform it back to TextSize. Is this ok?

I have just pushed the changes which show what I am referring to, I am happy to revert them in case 👍

arendjr · 2024-06-22T21:38:56Z

crates/biome_diagnostics/src/display/frame.rs

+        for (i, char) in iter {
+            let should_highlight = i >= marker.start().into() && i < marker.end().into();
+            if should_highlight {
+                fmt.write_markup(markup! { <Emphasis><Info>{char}</Info></Emphasis> })?;


Can we create subslices of adjacent characters that need highlighting, instead of adding the markup per character?

Absolutely! I have just pushed the changes, let me know if you have any feedback :)

arendjr · 2024-06-22T21:39:55Z

crates/biome_service/src/workspace/server.rs

@@ -783,10 +801,25 @@ impl Workspace for WorkspaceServer {
    }

    fn search_pattern(&self, params: SearchPatternParams) -> Result<SearchResults, WorkspaceError> {
+        let SearchPatternParams {
+            path,
+            pattern: _pattern,


You can use .. if you want to ignore additional fields.

ematipico

Since this PR doesn't add anything to the user (there's no command search yet), I think the changelog change should be reverted.

I also left some feedback and questions

ematipico · 2024-06-24T08:36:20Z

crates/biome_cli/src/execute/diagnostics.rs

@@ -103,6 +103,10 @@ pub(crate) struct UnhandledDiagnostic;
 #[diagnostic(category = "parse", message = "Skipped file with syntax errors")]
 pub(crate) struct SkippedDiagnostic;

+#[derive(Debug, Diagnostic)]
+#[diagnostic(category = "search", tags(SEARCH))]


Is there a particular reason why you added a SEARCH tag? Tags are meant to add additional information to the current diagnostic, and you can assign multiple tags. I'm not sure this tag fits in here, unless there's a reason I don't see

Just for my understanding, you think the category should be enough to right, and no tags would be needed here? If so, I guess that makes sense :)

Is there a particular reason why you added a SEARCH tag? Tags are meant to add additional information to the current diagnostic, and you can assign multiple tags. I'm not sure this tag fits in here, unless there's a reason I don't see

Yes, this is the reason why I went with it:

I think a SearchDiagnostic should have a Hint severity

The DiagnosticPrinter will deem a diagnostic skippable if its severity is below its diagnostic_level -- in our case, anything below Information is deemed skippable

For fn should_skip_diagnostic to print a search diagnostic despite its low severity, I figured I needed to have a SEARCH tag attached to the diagnostic_tags (see traverse.rs::should_skip_diagnostic)

With your previous comments, I realized I am able to get this info out of the printer execution if needed: so, if there are no better ways, I could change my code to query the execution if you'd want me to

Is there a particular reason why you added a SEARCH tag? Tags are meant to add additional information to the current diagnostic, and you can assign multiple tags. I'm not sure this tag fits in here, unless there's a reason I don't see

Yes, this is the reason why I went with it:

I think a SearchDiagnostic should have a Hint severity

The DiagnosticPrinter will deem a diagnostic skippable if its severity is below its diagnostic_level -- in our case, anything below Information is deemed skippable

For fn should_skip_diagnostic to print a search diagnostic despite its low severity, I figured I needed to have a SEARCH tag attached to the diagnostic_tags (see traverse.rs::should_skip_diagnostic)

With your previous comments, I realized I am able to get this info out of the printer execution if needed: so, if there are no better ways, I could change my code to query the execution if you'd want me to

I apologize for this confusion: after checking thoroughly, I realized this behavior was not happening the way I was imagining it.
I have now proceeded to remove the SEARCH tag and its associated checks altogether 👍

ematipico · 2024-06-24T08:37:00Z

crates/biome_cli/src/execute/process_file/search.rs

                .guard()
                .search_pattern(pattern)
                .with_file_path_and_code(
                    workspace_file.path.display().to_string(),
                    category!("search"),
                )?;

+            let input = workspace_file.input()?;
+            let file_name = workspace_file.path.display().to_string();
+
            // FIXME: We need to report some real results here...


Is the FIXME still relevant after this PR?

I guess so, since the formatting is ready for the engine that provides real results is :)

I guess so, since the formatting is ready for the engine that provides real results is :)

Sorry for my last out-of-sync reply: I actually went ahead and removed this one, in favor of keeping the other TODO inside the function which is actually supposed to return the matches

ematipico · 2024-06-24T08:43:25Z

crates/biome_cli/src/execute/process_file/search.rs

+                    .map(|mat| {
+                        SearchDiagnostic
+                            .with_file_span(mat)
+                            .with_severity(Severity::Hint)


If the severity of SearchDiagnostic doesn't depend on external factors (for example, the configuration), then the severity can be set directly in the definition of SearchDiagnostic:

#[derive(Debug, Diagnostic)] #[diagnostic(category = "search", tags(SEARCH), severity = Hint)] struct SearchDiagnostic;

Maybe Severity::Information is also more appropriate?

I think so too!

Sure! I will go ahead and change the severity to Information then

crates/biome_cli/src/reporter/terminal.rs

ematipico · 2024-06-24T09:31:30Z

crates/biome_service/src/workspace/server.rs

@@ -783,10 +801,22 @@ impl Workspace for WorkspaceServer {
    }

    fn search_pattern(&self, params: SearchPatternParams) -> Result<SearchResults, WorkspaceError> {
+        let SearchPatternParams { path, .. } = params;
+
        // FIXME: Let's implement some real matching here...


Is the FIXME still applicable? I see there's some new code, maybe the FIXME should become a TODO or something else?

A TODO definitely sounds better, thanks! 👍

arendjr · 2024-06-25T17:47:03Z

Thanks @BackupMiles !

BackupMiles added 4 commits June 22, 2024 16:09

feat(search): add search output formatter

4497e61

feat(search): add dummy data provider

c178fa7

ref(search): hide search

ce27943

Merge branch 'main' into backupmiles/feat/search-output-formatter

0e6202e

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Diagnostic Area: diagnostocis labels Jun 22, 2024

Conaclos requested a review from arendjr June 22, 2024 17:13

BackupMiles added 3 commits June 22, 2024 23:00

ref(search): format

47e9c21

Merge branch 'main' into backupmiles/feat/search-output-formatter

f6363d1

chore: update changelog

e85c34d

github-actions bot added the A-Changelog Area: changelog label Jun 22, 2024

fix: revert lock changes

b3c213c

arendjr approved these changes Jun 22, 2024

View reviewed changes

ref(search): remove unnecessary match

edeaf3a

ematipico self-requested a review June 23, 2024 16:14

BackupMiles added 2 commits June 23, 2024 18:28

ref(search): group range to highlight in markup

4eed44d

ref(search): remove unwanted fields

e603148

ematipico reviewed Jun 24, 2024

View reviewed changes

BackupMiles added 6 commits June 24, 2024 20:05

ref(search): mark matching spot as TODO

2280fb5

ref(search): remove explicit severity

7bf0e80

Merge branch 'main' into backupmiles/feat/search-output-formatter

da5dc0e

ref(search): change severity to information

bfd5ec6

ref(search): remove unnecessary tag

dc1e19c

chore(search): remove changelog line

b0f9948

github-actions bot removed the A-Changelog Area: changelog label Jun 24, 2024

ematipico approved these changes Jun 25, 2024

View reviewed changes

arendjr merged commit 50c880e into biomejs:main Jun 25, 2024
10 checks passed

ramzimalhas mentioned this pull request Aug 21, 2024

[Snyk] Upgrade @biomejs/biome from 1.7.1 to 1.8.3 ramzimalhas/popclipweb#3

Open

This was referenced Aug 28, 2024

[Snyk] Upgrade @biomejs/biome from 1.5.3 to 1.8.3 Ramyromel/mesc#2

Open

[Snyk] Upgrade @biomejs/biome from 1.5.3 to 1.8.3 Ramyromel/mesc#6

Open

This was referenced Sep 9, 2024

[Snyk] Upgrade: , vitest Ramyromel/mesc#8

Open

[Snyk] Upgrade: , , vitest Ramyromel/mesc#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(search-output-formatter): initialize search output formatter #3258

feat(search-output-formatter): initialize search output formatter #3258

BackupMiles commented Jun 22, 2024 •

edited

Loading

arendjr left a comment

arendjr Jun 22, 2024

BackupMiles Jun 22, 2024

arendjr Jun 22, 2024

arendjr Jun 22, 2024

BackupMiles Jun 23, 2024

BackupMiles Jun 23, 2024

arendjr Jun 22, 2024

BackupMiles Jun 24, 2024

arendjr Jun 22, 2024

ematipico left a comment

ematipico Jun 24, 2024

arendjr Jun 24, 2024

BackupMiles Jun 24, 2024 •

edited

Loading

BackupMiles Jun 24, 2024

ematipico Jun 24, 2024

arendjr Jun 24, 2024

BackupMiles Jun 24, 2024

ematipico Jun 24, 2024

arendjr Jun 24, 2024

ematipico Jun 24, 2024

BackupMiles Jun 24, 2024

ematipico Jun 24, 2024

BackupMiles Jun 24, 2024

arendjr commented Jun 25, 2024

feat(search-output-formatter): initialize search output formatter #3258

feat(search-output-formatter): initialize search output formatter #3258

Conversation

BackupMiles commented Jun 22, 2024 • edited Loading

Summary

Test Plan

arendjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BackupMiles Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arendjr commented Jun 25, 2024

BackupMiles commented Jun 22, 2024 •

edited

Loading

BackupMiles Jun 24, 2024 •

edited

Loading