Skip to content
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.

Sign up for GitHub

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

Jump to bottom

web: Add document highlight providers to API and extension host #11822

Merged
merged 23 commits into from Jul 2, 2020
Merged

web: Add document highlight providers to API and extension host #11822

merged 23 commits into from Jul 2, 2020

Conversation

efritz
Copy link
Contributor

@efritz efritz commented Jun 30, 2020 •
edited

Part of #10868. Sibling PRs: sourcegraph/code-intel-extensions#409 and sourcegraph/codeintellify#278

This PR adds the DocumentHighlightProvider to the extension API. The behavior is very straightforward, but there are lot of places that needed to be touched to thread things through to the leaves of the app appropriately. Please look with a close eye!

Kapture 2020-07-01 at 10 03 05

@efritz efritz changed the title Add document highlight providers web: Add document highlight providers to API and extension host Jul 1, 2020
This was referenced Jul 1, 2020
Merged
Merged
@efritz efritz marked this pull request as ready for review July 1, 2020 22:38
@efritz efritz requested a review from a team as a code owner July 1, 2020 22:38
@efritz efritz requested review from a team, felixfbecker and lguychard July 1, 2020 22:38
twop
twop reviewed Jul 1, 2020
View reviewed changes
Copy link
Contributor

@twop twop left a comment •
edited

Choose a reason for hiding this comment

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

Migration to RFC 155 architecture are tracked in #11879. Target is EOW but it seems you already used new architecture on the worker side. Overall changes look good to me
btw I think it makes sense to add unit tests for merging hovers and maybe one or two integrations tests here

package.json Outdated Show resolved Hide resolved
packages/sourcegraph-extension-api/src/sourcegraph.d.ts Show resolved Hide resolved
shared/src/api/client/services/completion.ts Outdated Show resolved Hide resolved
twop
twop suggested changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@twop twop left a comment

Choose a reason for hiding this comment

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

would be nice to have tests :)

  1. unit tests at least for merging results
  2. integration just to make sure it is hooked up e2e here
  3. UI tests maybe? Either via percy snapshots or react testing-library

@efritz
Copy link
Contributor Author

efritz commented Jul 2, 2020

unit tests at least for merging results

Done.

integration just to make sure it is hooked up e2e here

That only tests the old style services and I don't see integration tests for the new code host extension.

UI tests maybe? Either via percy snapshots or react testing-library

Could you guide me through a good way to do this/mock the required data?

@efritz efritz requested review from twop and sqs July 2, 2020 00:48
felixfbecker
felixfbecker reviewed Jul 2, 2020
View reviewed changes
Copy link
Contributor

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Looks good overall!

browser/src/app.scss Outdated Show resolved Hide resolved
browser/src/app.scss Outdated Show resolved Hide resolved
shared/src/api/contract.ts Outdated Show resolved Hide resolved
shared/src/api/extension/extensionHost.documentHighlights.test.ts Outdated Show resolved Hide resolved
shared/src/api/extension/extensionHost.ts Outdated Show resolved Hide resolved
web/src/SourcegraphWebApp.scss Outdated Show resolved Hide resolved
web/src/backend/features.ts Outdated Show resolved Hide resolved
shared/src/api/extension/extensionHost.documentHighlights.test.ts Outdated Show resolved Hide resolved
shared/src/api/extension/extensionHost.documentHighlights.test.ts Show resolved Hide resolved
@efritz efritz requested a review from felixfbecker July 2, 2020 17:15
felixfbecker
felixfbecker reviewed Jul 2, 2020
View reviewed changes
browser/src/app.scss Outdated Show resolved Hide resolved
@efritz @felixfbecker
Update browser/src/app.scss
eff270a 
Co-authored-by: Felix Becker <felix.b@outlook.com>
@efritz efritz requested a review from felixfbecker July 2, 2020 18:03
@efritz efritz requested a review from felixfbecker July 2, 2020 18:23
@codecov
Copy link

codecov bot commented Jul 2, 2020 •
edited

Codecov Report

Merging #11822 into master will decrease coverage by 2.14%.
The diff coverage is 55.00%.

@@            Coverage Diff             @@
##           master   #11822      +/-   ##
==========================================
- Coverage   49.92%   47.77%   -2.15%     
==========================================
  Files        1506     1402     -104     
  Lines       88470    79876    -8594     
  Branches     6837     6793      -44     
==========================================
- Hits        44165    38161    -6004     
+ Misses      40383    38140    -2243     
+ Partials     3922     3575     -347
Flag Coverage Δ
#go 51.95% <ø> (-2.37%) ⬇️
#storybook 10.09% <0.00%> (+0.06%) ⬆️
#typescript 36.65% <55.00%> (-0.13%) ⬇️
#unit 47.37% <55.00%> (-2.19%) ⬇️
Impacted Files Coverage Δ
browser/src/shared/code-hosts/shared/codeHost.tsx 57.40% <0.00%> (-1.64%) ⬇️
shared/src/api/extension/extensionHost.ts 76.81% <ø> (+2.94%) ⬆️
web/src/backend/features.ts 0.00% <0.00%> (ø)
...campaigns/detail/changesets/CampaignChangesets.tsx 60.00% <0.00%> (-0.38%) ⬇️
web/src/repo/blob/Blob.tsx 0.00% <0.00%> (-0.39%) ⬇️
web/src/repo/commit/RepositoryCommitPage.tsx 0.00% <0.00%> (-0.97%) ⬇️
web/src/repo/compare/RepositoryCompareArea.tsx 0.00% <0.00%> (ø)
shared/src/api/extension/api/documentHighlights.ts 100.00% <100.00%> (ø)
shared/src/api/extension/flatExtensionApi.ts 98.68% <100.00%> (+0.32%) ⬆️
cmd/frontend/db/orgs_mock.go 0.00% <0.00%> (-100.00%) ⬇️
... and 272 more

@efritz
Copy link
Contributor Author

efritz commented Jul 2, 2020

🎉 Tests pass!

felixfbecker
felixfbecker approved these changes Jul 2, 2020
View reviewed changes
web/src/SourcegraphWebApp.scss Outdated Show resolved Hide resolved
yarn.lock Show resolved Hide resolved
efritz and others added 4 commits July 2, 2020 15:23
@efritz @felixfbecker
Update web/src/SourcegraphWebApp.scss
93116f5 
Co-authored-by: Felix Becker <felix.b@outlook.com>
@efritz
Undo update.
71fae58
@efritz
Redo update.
fc7badd
@efritz
Merge branch 'document-highlight-provider' of github.com:sourcegraph/…
8b01544 
…sourcegraph into document-highlight-provider
@efritz efritz merged commit 2cc5e77 into master Jul 2, 2020
@efritz efritz deleted the document-highlight-provider branch July 2, 2020 20:37
@felixfbecker felixfbecker added the extensions Sourcegraph extensions label Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfbecker felixfbecker felixfbecker approved these changes

@lguychard lguychard Awaiting requested review from lguychard

@twop twop Awaiting requested review from twop

@sqs sqs Awaiting requested review from sqs

Assignees
No one assigned
Labels
extensions Sourcegraph extensions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@efritz @sqs @twop @felixfbecker