Show heading + color by helixbass · Pull Request #61 · helixbass/tree-sitter-grep

helixbass · 2023-07-18T02:53:24Z

In this PR:

mimic ripgrep's heading/filename/line/column formatting (at least much more closely), eg showing a per-file "heading" and colors by default when outputting to a terminal

To test:
Now when running in a terminal by default you should see ripgrep-style output formatting (but with a different default set of colors)
If you eg redirect the output by doing | cat -, you should see different formatting that is more like our existing formatting but without showing line numbers (by default)
You should be able to use the newly exposed command-line arguments to modify those behaviors, eg use --pretty when redirecting the output to still get color/headings etc, use --line-number when redirecting the output to still get line numbers

helixbass · 2023-07-18T02:53:49Z

 clap = { version = "4.3.0", features = ["derive", "wrap_help"] }
 encoding_rs = "0.8.14"
 encoding_rs_io = "0.1.6"
+grep-cli = "0.1.8"


This is another one of the ripgrep-derived crates

helixbass · 2023-07-18T02:54:57Z

 };

+#[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum)]
+pub enum ColorChoiceArg {


This is almost identical to the termcolor::ColorChoice that it ends up getting mapped to but I thought it made sense not to "conflate" those since (a) there is some mapping logic involved and (b) doing it this way we can tap into clap ValueEnum stuff

helixbass · 2023-07-18T02:55:54Z

+    ///
+    /// When the --vimgrep flag is given to tree-sitter-grep, then the default
+    /// value for the --color flag changes to 'never'.
+    #[arg(long, value_name = "WHEN"/*, default_value_t = ColorChoiceArg::Auto*/)]


When I tried to add the default value here it complained about Display not being implemented (not sure why exactly) so I just resolved to the default at the (single) point of usage instead

(also ended up running into the fact that the default is in fact not "static" (when in --vimgrep mode the default is ColorChoiceArg::Never instead) so using default_value_t would presumably not be appropriate, removing)

helixbass · 2023-07-18T02:57:14Z

+    #[arg(long)]
+    pub column: bool,
+
+    #[arg(long, hide = true, overrides_with = "column")]


The hide/overrides_with seem (per a little reading) to be the most reasonable way to do these --no-* argument variants

helixbass · 2023-07-18T02:58:03Z


 impl Args {
-    fn use_paths(&self) -> Vec<PathBuf> {
+    pub fn use_paths(&self) -> Vec<PathBuf> {


This is getting called in a couple different places now which seems like some unnecessary repeated allocations

So maybe I should make this effectively "memoized"

You could probably write a macro which does the once-cell thing that you want to the function it annotates?

I certainly like that concept

Here though my instinct (now that we're starting to venture into crate-level API territory) is that we should only "cache"/"memoize" at the level of this particular Args instance (vs my understanding would be that a "purely-declarative" once-cell-ish thing would have to be using something static in "scope" under the hood?)

And so then interesting to think if that version is also something that could be expressed declaratively/via a macro

I don't know, what I'm picturing now is just an interior-mutable "private" field on Args that we call eg .get_or_init() on?

I guess you could avoid writing the .get_or_init() boilerplate (so keeping the function body as just the "actual value-calculation logic") by writing a macro eg #[memoize_via_once_cell_field(name_of_once_cell_field)]?

Ah yeah, I didn't really think about how it'd have to be per-instance.

That seems like a reasonable approach to me (the once cell field). I wonder if clap exposes some option to call a constructor for Args so that you could at startup calculate this stuff rather than having to do it lazily. (It's not like it's not going to be used, so having to do work to make it smartly lazy seems kind of silly.)

helixbass · 2023-07-18T03:02:18Z

+            let path_stdin = Path::new("-");
+            self.with_filename
+                || self.vimgrep
+                || paths.len() > 1


And this is causing it to not show the filename in the match output when you just pass a single file path (which is what ripgrep does)

helixbass · 2023-07-18T03:03:30Z

    vec![
        #[cfg(unix)]
-        "path:fg:magenta".parse().unwrap(),
+        "path:fg:green".parse().unwrap(),


I forgot that I had ripgrep's default colors overridden in a local config file

I can't with the default ripgrep colors they're disgusting

So this is "my" color scheme (don't remember where I got it from or if I made it up)

I find the highlights pretty gross when running in my terminal using my color scheme (I assume this is somehow using the colors I have specified there?):

But it's readable 🤷🏻

Hmm ya they probably chose theirs out of trying to be light-background- vs dark-background-terminal-agnostic

But I have a hard time stomaching using their default ones

Maybe (a) a more minimal version as the default

And (b) supporting a ripgrep-style config file?

We could even be sneaky and (ultimately in addition to looking for "our own" config file) look for someone's actual ripgrep config file (on the assumption that they would like similar "defaults" applied here vs ripgrep)?

Either way the fact that with n = 2 we have different desired highlighting tells me that making it (easily, vs passing --colors every time you use it) configurable is worth doing?

Ha yes, 100% of people want it to be configurable, I guess, and I definitely couldn't be bothered to pass --colors every time I used it.

helixbass · 2023-07-18T03:04:47Z

        r#"
            $ tree-sitter-grep -q '(value_argument) @c' --language swift
-            example.swift:2:    atPath: "native"
+            example.swift:    atPath: "native"


Most of the existing test output changed because (a) those tests are running in "non-terminal-output" mode and (b) now in that mode we don't show line numbers by default (like ripgrep)

helixbass · 2023-07-18T03:06:13Z

-            src/lib.rs-6-
-            src/lib.rs-7-#[cfg(test)]
+            src/stop.rs:fn stop_it() {}
+            --


Per above here are some new -- separators between files when passing a "context" option

helixbass · 2023-07-18T03:07:34Z

        .stdout(predicate::function(|stdout: &str| {
            let stdout = massage_error_output(stdout);
+            if stdout != output {
+                print_diff(&stdout, &output, " ");


This was a hacky approximation of a "pretty assertion diff" that I'm kind of inclined to keep in (it only kicks in if the test is failing)?

What happens when color command codes are in the diff?

Mm not sure what that would show, this is only in the assert_non_match_output() helper (ie something that prints to stdout but what it prints isn't match output) so has pretty narrow usage, specifically I was using it to help me wrangle the --help output (which I don't think we are nor do I have a strong desire to test the "pretty" version of)

helixbass · 2023-07-18T13:53:33Z

      - run: cargo test
+        if: matrix.os == 'ubuntu-latest'
+        env:
+          TERM: linux


Apparently the TERM environment variable wasn't set at all in the Ubuntu CI environment so that was causing (per the --color docs) it to not print colors even when using eg --pretty which was causing tests to fail

So tried setting to what apparently is a pretty "generic" $TERM setting and it passed

That seems reasonable, but maybe that suggests that you should be able to control environment variables in tests?

Ya that could make sense

Here my specific understanding would be that then we'd have to eg "#[cfg(unix/windows)]-split" those particular tests (so that we could only set that environment variable for the Unix version), and would have to add that environment variable to every individual test that's using --pretty

To me that sounds clunkier vs putting it in the one place here that is basically saying "for all tests (on this platform) (by default) act like you're in this kind of underlying terminal environment"

helixbass · 2023-07-18T13:55:04Z

+    ///
+    /// When the --vimgrep flag is given to tree-sitter-grep, then the default
+    /// value for the --color flag changes to 'never'.
+    #[arg(long, value_name = "WHEN"/*, default_value_t = ColorChoiceArg::Auto*/)]


(also ended up running into the fact that the default is in fact not "static" (when in --vimgrep mode the default is ColorChoiceArg::Never instead) so using default_value_t would presumably not be appropriate, removing)

helixbass · 2023-07-18T14:02:38Z

+        "rust_project",
+        r#"
+             $ tree-sitter-grep -q '(function_item) @c' -l rust --pretty
+             [0m[1m[32msrc/stop.rs[0m


You could picture having a "cuter" test-syntax for this

But not sure it's worth it?

Surprisingly this passed on Windows too I guess they use the same terminal color escape codes or whatever?

Huh, I find that pretty surprising given how incompatible the terminals are in most other ways. (But they do somehow run git and whatever else, so maybe they are trying to be compatible.)

You could picture having a "cuter" test-syntax for this

I can't really 😄 — what were you thinking?

But not sure it's worth it?

Yeah, doesn't seem like it's worth a ton of effort.

Like some kind of "readable markup" of what the colors/styles should be?

Ya it might not be that cute anyway

helixbass · 2023-07-18T14:03:44Z


 fn strip_trailing_carriage_return(line: &str) -> Cow<'_, str> {
-    regex!(r#"\r$"#).replace(line, "")
+    regex!(r#"\r((?:\u{1b}\[\d+m)*)$"#).replace(line, "$1")


I was seeing terminal color escape codes after \r's (not sure if that's expected?) so this handles that

That's not where I'd have guessed they'd go, but why not 🤷🏻

helixbass · 2023-07-18T14:06:02Z


 fn normalize_match_path(line: &str) -> Cow<'_, str> {
-    regex!(r#"^[^:]+[:-]\d+[:-]"#)
+    regex!(r#"^(?:\u{1b}\[\d+m)*[a-zA-Z_\-\\/]+\.(?:rs|js|tsx)"#)


And here this regex for finding "paths" (for changing \ -> / on Windows) keeps getting re-worked, now it really can't really on "context" at all (since eg heading lines are just paths by themselves)

So just expressed it in a way that works for all current tests with filename output

peterstuart

Nice, looks good 👍🏻

peterstuart · 2023-07-20T21:08:38Z

      - run: cargo test
+        if: matrix.os == 'ubuntu-latest'
+        env:
+          TERM: linux


That seems reasonable, but maybe that suggests that you should be able to control environment variables in tests?

peterstuart · 2023-07-20T21:09:42Z

+    /// will be shown as a prefix for each matched line.
+    ///
+    /// This flag overrides --no-filename.
+    #[arg(short = 'H', long)]


Assuming you took this from ripgrep, but what's the "H" mean? "heading-ish"?

Good question. At some point their algorithm may be "if we start running out of the "obvious short codes" then give the "less obvious" ones to less-used options"?

peterstuart · 2023-07-20T21:11:18Z


 impl Args {
-    fn use_paths(&self) -> Vec<PathBuf> {
+    pub fn use_paths(&self) -> Vec<PathBuf> {


You could probably write a macro which does the once-cell thing that you want to the function it annotates?

peterstuart · 2023-07-20T21:13:45Z

    vec![
        #[cfg(unix)]
-        "path:fg:magenta".parse().unwrap(),
+        "path:fg:green".parse().unwrap(),


I find the highlights pretty gross when running in my terminal using my color scheme (I assume this is somehow using the colors I have specified there?):

But it's readable 🤷🏻

peterstuart · 2023-07-20T21:15:26Z

+        "rust_project",
+        r#"
+             $ tree-sitter-grep -q '(function_item) @c' -l rust --pretty
+             [0m[1m[32msrc/stop.rs[0m


Huh, I find that pretty surprising given how incompatible the terminals are in most other ways. (But they do somehow run git and whatever else, so maybe they are trying to be compatible.)

peterstuart · 2023-07-20T21:16:28Z

+        "rust_project",
+        r#"
+             $ tree-sitter-grep -q '(function_item) @c' -l rust --pretty
+             [0m[1m[32msrc/stop.rs[0m


You could picture having a "cuter" test-syntax for this

I can't really 😄 — what were you thinking?

But not sure it's worth it?

Yeah, doesn't seem like it's worth a ton of effort.

peterstuart · 2023-07-20T21:17:23Z


 fn strip_trailing_carriage_return(line: &str) -> Cow<'_, str> {
-    regex!(r#"\r$"#).replace(line, "")
+    regex!(r#"\r((?:\u{1b}\[\d+m)*)$"#).replace(line, "$1")


That's not where I'd have guessed they'd go, but why not 🤷🏻

peterstuart · 2023-07-20T21:18:40Z

        .stdout(predicate::function(|stdout: &str| {
            let stdout = massage_error_output(stdout);
+            if stdout != output {
+                print_diff(&stdout, &output, " ");


What happens when color command codes are in the diff?

helixbass added 6 commits July 17, 2023 20:45

working on heading

e745f3d

existing tests passing

f7c9965

show no heading

c0e303f

with filename

a9a06c0

line number

3305c65

existing tests passing

1c79bab

helixbass commented Jul 18, 2023

View reviewed changes

helixbass added 15 commits July 18, 2023 07:54

some tests

816284b

windows ci

7db3468

windows ci

256ef80

vimgrep default color

c9d3e19

test color

656355a

windows

5d2c4ff

windows tests

90780a4

windows tests

ec92240

windows tests

46abf52

windows tests

86b1fe0

tests

e2c89e2

debug CI

2a5f886

debug CI

0bf3726

try setting TERM for ci

40efe11

restore test

ffaab52

helixbass changed the title ~~WIP: show heading + color~~ Show heading + color Jul 18, 2023

helixbass added 2 commits July 18, 2023 09:52

only set conditionally

a080287

rm unused

c64af45

helixbass commented Jul 18, 2023

View reviewed changes

peterstuart approved these changes Jul 20, 2023

View reviewed changes

Conversation

helixbass commented Jul 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterstuart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!