search: rename `repo:contains` predicates to be consistent by tbliu98 · Pull Request #40389 · sourcegraph/sourcegraph-public-snapshot

tbliu98 · 2022-08-15T21:55:45Z

Part of #39767

As described in the linked issue, this PR:

Renames repo:contains(file:a content:b) to repo:contains.file(path:a content:b)
Renames repo:contains.file(a) to repo:contains.path(a)
Removes support for repo:contains(file:a content:b)

Note that if we think keeping repo:contains around is actually the way to go, it will be easy to add it back.

This PR includes corresponding frontend changes to syntax highlighting, hovers, and the search reference panel.

Test plan

Updated unit tests. Integration tests. Manually tested hovers/suggestions/syntax highlighting/reference panel

…ontent:b); rename repo:contains.file(a) to repo:contains.path(a)

tbliu98 · 2022-08-15T21:58:04Z

-            'Search only inside repositories that contain a file matching the `file:` with `content:` filters.',
-        examples: ['repo:contains(file:CHANGELOG content:fix)'],
+            'Search only inside repositories that contain a file path matching the `path:` and/or `content:` filters.',
+        examples: ['repo:contains.file(path:README content:fix)'],


repo:contains.file(path:CHANGELOG content:fix) was too long to fit fully into the sidebar. I don't know how line wrapping is handled in the sidebar but it was easiest to just shorten the example string.

…epo:contains.file` and `repo:contains.path`

tbliu98 · 2022-08-19T01:40:36Z

+    // repo:contains() is not supported, use repo:contains.file() or repo:contains.path()
+    if (path.length === 1 && path[0] === 'contains') {
+        return undefined
+    }


This is to disable syntax highlighting for repo:contains() to make it visually clear it's not valid anymore. This feels like a band-aid but seemed like the least disruptive way to achieve that end.

Ended up reverting this change because it affected file:contains.content. I still want to disable syntax highlighting for repo:contains() but will handle that in a follow-up to limit the scope of this PR.

This reverts commit d389e92.

sourcegraph-bot · 2022-08-19T02:47:11Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff e0289af...d44ee77.

Notify	File(s)
@beyang	internal/search/job/jobutil/job_test.go internal/search/query/parser_test.go internal/search/query/predicate.go internal/search/query/predicate_test.go internal/search/query/query_test.go internal/search/query/testdata/TestSubstituteSearchContexts/preserve_predicate_label.golden internal/search/query/types.go internal/search/query/visitor_test.go
@camdencheek	internal/search/job/jobutil/job_test.go internal/search/query/parser_test.go internal/search/query/predicate.go internal/search/query/predicate_test.go internal/search/query/query_test.go internal/search/query/testdata/TestSubstituteSearchContexts/preserve_predicate_label.golden internal/search/query/types.go internal/search/query/visitor_test.go
@fkling	client/search-ui/src/results/sidebar/SearchReference.tsx client/shared/src/search/query/completion.test.ts client/shared/src/search/query/decoratedToken.test.ts client/shared/src/search/query/decoratedToken.ts client/shared/src/search/query/hover.test.ts client/shared/src/search/query/hover.ts client/shared/src/search/query/predicates.test.ts client/shared/src/search/query/predicates.ts client/shared/src/search/query/scanner.test.ts
@keegancsmith	internal/search/job/jobutil/job_test.go internal/search/query/parser_test.go internal/search/query/predicate.go internal/search/query/predicate_test.go internal/search/query/query_test.go internal/search/query/testdata/TestSubstituteSearchContexts/preserve_predicate_label.golden internal/search/query/types.go internal/search/query/visitor_test.go
@limitedmage	client/search-ui/src/results/sidebar/SearchReference.tsx
@rvantonder	client/shared/src/search/query/completion.test.ts client/shared/src/search/query/decoratedToken.test.ts client/shared/src/search/query/decoratedToken.ts client/shared/src/search/query/hover.test.ts client/shared/src/search/query/hover.ts client/shared/src/search/query/predicates.test.ts client/shared/src/search/query/predicates.ts client/shared/src/search/query/scanner.test.ts internal/search/query/parser_test.go internal/search/query/predicate.go internal/search/query/predicate_test.go internal/search/query/query_test.go internal/search/query/testdata/TestSubstituteSearchContexts/preserve_predicate_label.golden internal/search/query/types.go internal/search/query/visitor_test.go
@sourcegraph/code-insights-backend	enterprise/internal/insights/query/querybuilder/builder_test.go
@unknwon	dev/gqltest/search_test.go

rvantonder

stamping to unblock since this is a mechanical change :-)

pls add changelog entry if I missed it

tbliu98 · 2022-08-19T03:10:06Z

Integration tests failing, will look tomorrow, it's late here
Will add changelog entry also, ty for reminder

coury-clark · 2022-08-19T03:23:39Z

Am I correct in assuming this is not backwards compatible? If so, that's going to be a problem for products with persisted queries such as Code Insights, without either some effort to migrate those queries or time to notify customers. Maybe I'm missing something here though, please let me know.

cc @Joelkw

rvantonder · 2022-08-19T04:12:49Z

@coury-clark IIRC code insights enforces a single repo (or maybe custom set of repos) and doesn't support repo regex/predicates?

If it does support anything beyond single or discrete multi-repo set then we should call this out as a breaking change in 4.0 (in practical terms I suspect there is a <1% chance that queries using that specific syntax persist (see original issue for syntax changes), and the burden of maintaining backwards compatibility is not worth the effort on our side. I.e., we are best off declaraing a breaking change in a major version version release that is unlikely to practically impact users, than ensuring backwars compatibility)

coury-clark · 2022-08-19T17:20:20Z

@rvantonder You're right, there are some light validations after certain versions to exclude executing repo: (this is why I shouldn't open PRs at night 🤦 ). We don't necessarily have a way to say for sure a customer isn't using a query like this without some kind of migration. If the usage numbers are expected to be that low, and we don't always accept this input then I agree the risk for Code Insights is minimal.

Having said that, other products / usage exist (Notebooks, monitors, batch changes, etc) as well as just general usage such as links. I think that in general a breaking change in our syntax deserves at least some lead time and notice, even if usage is low. Apologies if I missed something, but I didn't see any kind of notice for this change either internally or customer facing.

tbliu98 · 2022-08-19T17:57:27Z

@coury-clark No we didn't send a department-wide notice regarding our plans to make this change. It should have occurred to me to do that more proactively, so my bad there.

That said, I haven't actually formally requested review from anyone on this PR yet, so I will go ahead and tag backend-devs on this now for visibility. I'm fine with holding off on merging this PR if others raise concerns. But, I also lean toward the side of thinking the impact of this will not be that large.

tbliu98 · 2022-08-22T16:46:31Z

Will go ahead with merging this later today if there are no objections

cc @sourcegraph/search-product

rvantonder · 2022-08-22T16:53:58Z

send a department-wide notice regarding our plans to make this change

@tbliu98 can you post this in #search, then cross share to #eng-announce, and ask anyone to raise concerns in the thread? This will give visibility to CEs, etc.

Rename repo:contains(file:a content:b) to repo:contains.file(path:a c…

f0054a2

…ontent:b); rename repo:contains.file(a) to repo:contains.path(a)

cla-bot Bot added the cla-signed label Aug 15, 2022

tbliu98 commented Aug 15, 2022

View reviewed changes

tbliu98 force-pushed the tl/rename-repo-contains-preds branch 2 times, most recently from c0d0b20 to 8ed6157 Compare August 19, 2022 00:15

Update syntax highlighting, hovers, and search reference panel for `r…

71c452b

…epo:contains.file` and `repo:contains.path`

tbliu98 force-pushed the tl/rename-repo-contains-preds branch from 8ed6157 to 71c452b Compare August 19, 2022 00:21

fix test

de85328

tbliu98 force-pushed the tl/rename-repo-contains-preds branch from 6f345ff to de85328 Compare August 19, 2022 01:15

tbliu98 added 2 commits August 18, 2022 20:33

add 'path' to path-like values

c534a12

disable syntax highlighting for repo:contains()

d389e92

tbliu98 commented Aug 19, 2022

View reviewed changes

tbliu98 added 3 commits August 18, 2022 20:57

update scanner.test.ts

b438c7e

Revert "disable syntax highlighting for repo:contains()"

a8242c5

This reverts commit d389e92.

update predicates.test.ts

2865b60

tbliu98 marked this pull request as ready for review August 19, 2022 02:45

rvantonder approved these changes Aug 19, 2022

View reviewed changes

update integration tests

d023ba1

tbliu98 requested a review from a team August 19, 2022 17:57

update CHANGELOG.md

d44ee77

tbliu98 force-pushed the tl/rename-repo-contains-preds branch from 1eecbd9 to d44ee77 Compare August 22, 2022 16:42

tbliu98 merged commit bef1519 into main Aug 23, 2022

tbliu98 deleted the tl/rename-repo-contains-preds branch August 23, 2022 16:24

tbliu98 mentioned this pull request Aug 23, 2022

Fix tests affected by changes to repo:contains.file syntax #40752

Merged

jtibshirani mentioned this pull request Jan 27, 2023

Search: restore old syntax for repo:contains filters #47007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

search: rename `repo:contains` predicates to be consistent#40389

search: rename `repo:contains` predicates to be consistent#40389
tbliu98 merged 10 commits into
mainfrom
tl/rename-repo-contains-preds

tbliu98 commented Aug 15, 2022 •

edited

Loading

Uh oh!

tbliu98 Aug 15, 2022 •

edited

Loading

Uh oh!

tbliu98 Aug 19, 2022

Uh oh!

tbliu98 Aug 19, 2022

Uh oh!

sourcegraph-bot commented Aug 19, 2022 •

edited

Loading

Uh oh!

rvantonder left a comment

Uh oh!

tbliu98 commented Aug 19, 2022

Uh oh!

coury-clark commented Aug 19, 2022

Uh oh!

rvantonder commented Aug 19, 2022

Uh oh!

coury-clark commented Aug 19, 2022

Uh oh!

tbliu98 commented Aug 19, 2022 •

edited

Loading

Uh oh!

tbliu98 commented Aug 22, 2022 •

edited

Loading

Uh oh!

rvantonder commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

tbliu98 commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

tbliu98 Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbliu98 Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

tbliu98 Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

sourcegraph-bot commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvantonder left a comment

Choose a reason for hiding this comment

Uh oh!

tbliu98 commented Aug 19, 2022

Uh oh!

coury-clark commented Aug 19, 2022

Uh oh!

rvantonder commented Aug 19, 2022

Uh oh!

coury-clark commented Aug 19, 2022

Uh oh!

tbliu98 commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbliu98 commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvantonder commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tbliu98 commented Aug 15, 2022 •

edited

Loading

tbliu98 Aug 15, 2022 •

edited

Loading

sourcegraph-bot commented Aug 19, 2022 •

edited

Loading

tbliu98 commented Aug 19, 2022 •

edited

Loading

tbliu98 commented Aug 22, 2022 •

edited

Loading