Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Conversation

@ijsnow
Copy link
Contributor

@ijsnow ijsnow commented Aug 23, 2018

Sometimes language servers return invalid hover information. Currently, codeintellify just sort of gets angry and quits working after this happens. @chrismwendt and I discussed a few different options for handling this and we decided that while the solution this PR has is the ideal solution1, it is more convenient for consumers of codeintellify.

1 The ideal solution would be that each language server perfectly adheres to the specification or at least each codeintellify consumer handles the errors on it's own.

@ijsnow ijsnow requested a review from felixfbecker August 23, 2018 02:22
@felixfbecker
Copy link
Contributor

Which consumers are we optimising for? Ourselves, or community usage? For the community, it would be nice if we could still highlight that wrong hovers are returned, otherwise these mistakes may never get caught and other clients suffer.

We have code in the webapp that handles more cases than just Python:

https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/blob/web/src/backend/lsp.tsx#L119-141
https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/blob/web/src/backend/lsp.tsx#L173-177

Currently, codeintellify just sort of gets angry and quits working after this happens.

If we adopt the above code (throwing an error if validation fails) it should only show an error hover with "invalid hover returned"

@ijsnow
Copy link
Contributor Author

ijsnow commented Aug 23, 2018

This is not optimizing for a subset of consumers. Now when any hover is formatted incorrectly, the error that we map to is displayed.

@codecov
Copy link

codecov bot commented Aug 23, 2018

Codecov Report

Merging #47 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage   65.14%   65.14%   +<.01%     
==========================================
  Files          15       15              
  Lines         634      637       +3     
  Branches      169      172       +3     
==========================================
+ Hits          413      415       +2     
- Misses        221      222       +1
Impacted Files Coverage Δ
src/hoverifier.ts 56.14% <66.66%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7fa415...a0a5c99. Read the comment docs.

@ijsnow
Copy link
Contributor Author

ijsnow commented Aug 23, 2018

At my computer now so I can give a more complete response. The fact that there is code to handle this in the web app and not the browser extension is exactly the point of this PR. Additionally, the code linked above would just hide these errors.

The error we map to when !HoverMerged.is(hoverMergedOrNull), is Invalid hover response which is 100% accurate according to the typings at that point. It should either be a valid HoverMerged or null and if its not, we should raise an error, not just correct the shape and pass it on with empty data.

@ijsnow ijsnow merged commit 6630cf9 into master Aug 23, 2018
@sourcegraph-bot
Copy link

🎉 This PR is included in version 3.7.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ijsnow ijsnow deleted the invalid-hovers branch August 23, 2018 22:19
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.

4 participants