Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Search backend: make isGlobal operate on search.RepoOptions#40236

Merged
camdencheek merged 5 commits into
mainfrom
cc/global-refactor
Aug 12, 2022
Merged

Search backend: make isGlobal operate on search.RepoOptions#40236
camdencheek merged 5 commits into
mainfrom
cc/global-refactor

Conversation

@camdencheek

@camdencheek camdencheek commented Aug 10, 2022

Copy link
Copy Markdown
Member

This is a followup from this comment.

Basically, isGlobal should operate on the RepoOptions type rather than the raw query now that the repo options type has all information about what repositories should be searched.

isGlobal is not a method on RepoOptions because our backend packages are too intertwined and this causes circular import issues.

As part of this, I documented that method heavily because I've historically been quite confused about its behavior.

Test plan

Depending on existing tests. Should be semantics-preserving -- it's just moving logic around.

@cla-bot cla-bot Bot added the cla-signed label Aug 10, 2022
@camdencheek camdencheek requested a review from a team August 10, 2022 23:33
@sourcegraph-bot

sourcegraph-bot commented Aug 10, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6cf2456...9ee745e.

Notify File(s)
@beyang internal/search/job/jobutil/job.go
internal/search/types.go
@keegancsmith internal/search/job/jobutil/job.go
internal/search/types.go

@camdencheek camdencheek removed the request for review from a team August 11, 2022 00:05
@camdencheek camdencheek marked this pull request as draft August 11, 2022 00:05
@camdencheek

Copy link
Copy Markdown
Member Author

Converted to draft because building stuff on top of this was causing circular import issues.

@camdencheek camdencheek marked this pull request as ready for review August 11, 2022 01:04
@camdencheek camdencheek changed the title Search backend: move IsGlobal logic into the zoekt package Search backend: make isGlobal operate on search.RepoOptions Aug 11, 2022
@camdencheek camdencheek requested a review from a team August 11, 2022 01:05
@camdencheek camdencheek added the team/search-product Issues owned by the search product team label Aug 11, 2022
@rvantonder rvantonder requested a review from novoselrok August 12, 2022 03:20

@rvantonder rvantonder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve if it's been run against backend-integration

isEmpty := noPattern && noFile && noLang

repoUniverseSearch = isGlobalSearch() && isIndexedSearch && hasGlobalSearchResultType && !isEmpty
repoUniverseSearch = isGlobalSearch && isIndexedSearch && hasGlobalSearchResultType && !isEmpty

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: maybe st != query.SearchTypeStructural is better as a part of this condition, or the hasGlobalSearchResultType check. then this conditional doesn't hang onto isGlobalSearch. can do in a follow up later though, if tests pass already on this PR just merge don't rerun (was this run against backend-integration or main? don't see it in the PR name

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, a good followup. This specific PR didn't run backend integration, but there are 3 PRs stacked on top of it that have.

@camdencheek camdencheek merged commit 10a9ad6 into main Aug 12, 2022
@camdencheek camdencheek deleted the cc/global-refactor branch August 12, 2022 19:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/search-product Issues owned by the search product team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants