-
-
Notifications
You must be signed in to change notification settings - Fork 367
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.
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
Upgrade to new version of lsp libraries #2494
Changes from 4 commits
5940164
e4c25b4
6f43acb
1d94ed5
35c8ef5
26b89d8
0bf7fee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,13 +46,7 @@ moduleOutline ideState DocumentSymbolParams{ _textDocument = TextDocumentIdentif | |
(defDocumentSymbol l :: DocumentSymbol) | ||
{ _name = pprText m | ||
, _kind = SkFile | ||
, _range = Range (Position 0 0) (Position 2147483647 0) -- _ltop is 0 0 0 0 | ||
-- In the lsp spec from 3.16 Position takes a uinteger, | ||
-- where uinteger is 0 - 2^31 - 1. lsp-types currently has the type of line | ||
-- as Int. So instead of using `maxBound :: Int` we hardcode the maxBound of | ||
-- uinteger. 2 ^ 31 - 1 == 2147483647 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh oh. I think this means I got it wrong: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made them an issue: microsoft/language-server-protocol#1394 But in the short term this kind of sucks... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I can go back and use a custom type instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ouch, but we can go ahead with this lsp version? or you want to redefine the type in lso before the release There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice not to release a version of I think the best thing might be to change to a custom type in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm but if we use UInt31 in LSP with all the required intances, including from integral, would no be matter of replacing Word32 with UInt31 here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's basically it. I just need to do it. I'll try and find time tomorrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe with a more generic name like LSPUInt? not sure if the data type could even change in the spec but I can imagine it could be bigger There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
-- Check this issue for tracking https://github.com/haskell/lsp/issues/354 | ||
-- the change in lsp-types. | ||
, _range = Range (Position 0 0) (Position maxBound 0) -- _ltop is 0 0 0 0 | ||
} | ||
_ -> Nothing | ||
importSymbols = maybe [] pure $ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will currently wrap. Should we do something else here? Like take
max 0
first?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah not sure if we have another sensible alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. I think either let it wrap or max with 0.