-
Notifications
You must be signed in to change notification settings - Fork 64
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
search: use chunk matches to render multiline streamed results #807
Conversation
0f912e2
to
019dfe4
Compare
019dfe4
to
d1c5991
Compare
|
||
[38;5;2msourcegraph/sourcegraph[0m[38;5;239m ([0m[38;5;23mhttp://127.0.0.1:55128/sourcegraph/sourcegraph[0m[38;5;239m)[0m[38;5;2m (1 match)[0m | ||
[0m [38;5;69m 5[0m[38;5;239m | [0m[38;5;0m[48;5;11mfoo[0m bar [38;5;0m[48;5;11mfoo[0m[38;5;239m | ||
------------------------------------------------------------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a minor cosmetic change, that we will put a ----
line below the last result, and is a consequence of keeping the loop logic simple
d1c5991
to
e47eaa2
Compare
If we version src-cli in tandem with sourcegraph this should be safe. If not, I worry that a customer is on an older version of sourcegraph and now src search -stream breaks. Either way we need to document this loudly in the changelog. I know of customers using the json output with streaming.
Sounds good to me! Additionally can we update the output of this so its less janky? I think you have done some of that, but man I really wish it didn't automatically have color output and the use of a pager. When using src-cli I am nearly always wanting to consume it via a tool and not as a human. |
@camdencheek nice catch, I remembered the part about overlapping ranges but naively thought it would unset and just re-enable highlighting. A test case proved otherwise. I implemented a test and the counter idea to fix this @keegancsmith we are set with in-tandem compatibility. Is bold markdown enough for "loudly" communicating this or should we go harder with
Unfortunately I have developed an allergy for drive-by requests and will have to decline 😛 I really just want to make sure we benefit from @camdencheek's hard work while it's fresh, and expedite our ability to get rid of |
Alright I'm going to go ahead with merging this. I made the changelog entry louder. I will triage client/customer usage issues coming from this change (so you know who to call). I suspect this is not going to be a high impact change. We really have to move forward at some point and break away from broken stuff for our own sanity. |
Also I checked the docs to see if this change should update anything there, and instead see we're still not guaranteeing backward compatibility anyway.
|
This switches
src search -stream 'query'
to use the chunk match result type.This means:
Accurate result counts for patterns that match across newlines
Multiline/structural search highlighting is now accurate (previously structural search was highlighting was broken)
New JSON schema for
src search -stream -json 'query'
new schema
old schema
PR details:
I am making the decision to remove line matches entirely for this option, because otherwise we can never move away from line matches/
preview
to adopt chunk matches as the only source of truth, which we really should do. It is possible that this could break clients/consumers that expect line matches for the-stream -json
option, and the changelog is updated accordingly.Maybe this should be a major version bump, but looks like we keep src-cli releases in tandem with Sourcegraph? Or is this minor enough that we can ship this with a minor
src
release (to what extent have we encouraged usingsearch -stream
? consumers could in theory remove-stream
and revert to using GQL to regain "old" behavior). If we don't feel comfortable with removingLineMatches
in this PR, I would prefer holding out until 4.0 release so I can have the opportunity to piggy back on the "breaking change" version bump.I think the follow up we want is:
search -stream
the defaultsearch
search -gql
instead (only useful for clients who want to get JSON I imagine).Test plan
Updates tests