Skip to content
This repository has been archived by the owner on Mar 18, 2024. It is now read-only.

Scope DocumentSelector with baseUri #303

Merged
merged 2 commits into from
Mar 8, 2020
Merged

Scope DocumentSelector with baseUri #303

merged 2 commits into from
Mar 8, 2020

Conversation

felixfbecker
Copy link
Contributor

@felixfbecker felixfbecker commented Mar 6, 2020

This is a proper fix for both https://github.com/sourcegraph/sourcegraph/issues/8602 and broken codeintel for root files (Slack thread).
Codeintel extensions need to be able to scope providers to workspace roots. Doing so in pattern does not work, because minimatch is not built for working with URIs (in particular it broke for files in the root).

Instead, uses the new baseUri introduced in https://github.com/sourcegraph/sourcegraph/pull/8858.

Closes #295.
Fixes sourcegraph/sourcegraph#8602.

@felixfbecker felixfbecker force-pushed the base-uri branch 4 times, most recently from 3f08bbb to 3015548 Compare March 6, 2020 23:23
@felixfbecker felixfbecker marked this pull request as ready for review March 6, 2020 23:24
@felixfbecker felixfbecker requested a review from a team as a code owner March 6, 2020 23:24
@efritz
Copy link
Contributor

efritz commented Mar 6, 2020

Am I correct that publishing this will break all current instances?

@felixfbecker
Copy link
Contributor Author

No, it should not. Current instances will ignore the baseUri property. They will just continue to see the double hovers unfortunately until they upgrade to 3.14.

@efritz
Copy link
Contributor

efritz commented Mar 6, 2020

But we’re no longer setting pattern so won’t no providers be registered correctly on old instances?

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Mar 6, 2020

But the pattern was just root URI + ** before (i.e. all files), and we're still setting language. If the language server provided a pattern we're still passing that through (just "unscoped"), but none of our language servers use that afaik.

@felixfbecker
Copy link
Contributor Author

(please sanity check what I'm saying, it's late 😅 )

@efritz
Copy link
Contributor

efritz commented Mar 6, 2020

Go to bed I’ll test it later tonight and merge

@efritz
Copy link
Contributor

efritz commented Mar 7, 2020

I ran this and it works neither with DotCom (#8858 has been merged by now and the site version has a rev after that merge commit) nor with a private instance before the merge.

Copy link
Contributor

@efritz efritz left a comment

Choose a reason for hiding this comment

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

Needs more manual testing.

@felixfbecker
Copy link
Contributor Author

I'm very confused. I thought the Go issue were fixed by #292 (the revert), and we only had double hovers now?

@felixfbecker
Copy link
Contributor Author

I found the issue, fix in https://github.com/sourcegraph/sourcegraph/pull/8877. The issue was in the extension host, i.e. the changes in this PR are correct.

Verified that codeintel works:

image

@efritz efritz merged commit a1c5027 into master Mar 8, 2020
@efritz efritz deleted the base-uri branch March 8, 2020 20:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate type signatures/docs in hover tooltips
2 participants