Fix (graphql-language-service) Hover for first line#4162
Fix (graphql-language-service) Hover for first line#4162lesleydreyer wants to merge 7 commits intographql:mainfrom
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4162 +/- ##
==========================================
+ Coverage 63.96% 64.02% +0.05%
==========================================
Files 35 35
Lines 3086 3091 +5
Branches 949 949
==========================================
+ Hits 1974 1979 +5
Misses 1107 1107
Partials 5 5
🚀 New features to boost your workflow:
|
trevor-scheer
left a comment
There was a problem hiding this comment.
Thanks for the PR @lesleydreyer! At some point I'll have a closer look at this, but I left a couple comments in case you feel like poking around a bit more to get this right. Hopefully I'll have some more helpful comments for you in the near future.
| end: { character: 23, line: -1 }, | ||
| start: { character: 22, line: -1 }, |
There was a problem hiding this comment.
This smells funny, I suspect we need to normalize the off by 1 line number at the LSP/Monaco boundary where the line off by 1 is happening (presumably).
I can spend some more time investigating at some point but there's probably a more comprehensive solution that results in line numbers we'd actually expect here (presumably 0).
| // LSP may count from 1, but monaco counts from 0 | ||
| for ( | ||
| let i = 0; | ||
| location.line === 0 ? i <= location.line : i < location.line; |
There was a problem hiding this comment.
Related to the comment above, this doesn't feel like the correct place for this fix.
fix hover for line 1 (0) in graphiql -> monaco-graphiql when hovering the first line
closes #4157 closes #3168