-
Notifications
You must be signed in to change notification settings - Fork 94
Fix for #374 #376
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
Fix for #374 #376
Conversation
Are you going to try and tackle the fix too, or is this just to help someone else trying to do that? |
Plan to follow up with a fix eventually |
ea4f26c
to
ee623dd
Compare
ee623dd
to
32593ee
Compare
It would be a good idea to test this change against the HLS test suite before merging. |
Good thing I have a branch getting HLS to build with the latest LSP :) |
@@ -354,7 +354,10 @@ updateState (FromServerMess SWorkspaceApplyEdit r) = do | |||
allChangeParams <- case r ^. params . edit . documentChanges of | |||
Just (List cs) -> do | |||
mapM_ (checkIfNeedsOpened . documentChangeUri) cs | |||
return $ mapMaybe getParamsFromDocumentChange cs | |||
-- replace the user provided version numbers with the VFS ones + 1 |
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.
why? are the user-provided ones wrong?
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.
the user can target a version older than the current one
@@ -412,6 +414,8 @@ updateState (FromServerMess SWorkspaceApplyEdit r) = do | |||
getParamsFromDocumentChange (InL textDocumentEdit) = Just $ getParamsFromTextDocumentEdit textDocumentEdit | |||
getParamsFromDocumentChange _ = Nothing | |||
|
|||
bumpNewestVersion (VersionedTextDocumentIdentifier uri _) = |
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.
signature please
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.
I'd have to write that type signature myself, without the help of the IDE!! Unacceptable
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
Testing in haskell/haskell-language-server#2494 |
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
All tests passing now.. I found 2 completions tests in the func-test suite that were using I think we should merge this before the release |
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.
If you've tested it, I'm happy. I don't 100% understand what's going on here, but I trust that you're doing something sensible.
* Update to latest version of lsp libraries * Compute completions on kick This is not only for faster completions. It's also needed to have semi-fresh completions after editing. This is specially important for the first completion request of a file - without this change there are no completions available at all * Emit LSP custom messages on kick start/finish useful to synchonize on these events in tests * Fix completions tests after haskell/lsp#376 * Restore cabal update with comments * Use new lsp in stack 9.0.1 Co-authored-by: Pepe Iborra <pepeiborra@gmail.com> Co-authored-by: jneira <atreyu.bbb@gmail.com>
* Update to latest version of lsp libraries * Compute completions on kick This is not only for faster completions. It's also needed to have semi-fresh completions after editing. This is specially important for the first completion request of a file - without this change there are no completions available at all * Emit LSP custom messages on kick start/finish useful to synchonize on these events in tests * Fix completions tests after haskell/lsp#376 * Restore cabal update with comments * Use new lsp in stack 9.0.1 Co-authored-by: Pepe Iborra <pepeiborra@gmail.com> Co-authored-by: jneira <atreyu.bbb@gmail.com> fix merge failure
The problem only appears for
documentChanges
edits.