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.

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

Merged
merged 5 commits into from
Dec 20, 2021
Merged

Fix for #374 #376

merged 5 commits into from
Dec 20, 2021

Conversation

pepeiborra
Copy link
Collaborator

The problem only appears for documentChanges edits.

@michaelpj
Copy link
Collaborator

Are you going to try and tackle the fix too, or is this just to help someone else trying to do that?

@pepeiborra
Copy link
Collaborator Author

Plan to follow up with a fix eventually

@pepeiborra
Copy link
Collaborator Author

It would be a good idea to test this change against the HLS test suite before merging.

@michaelpj
Copy link
Collaborator

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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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 _) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

signature please

Copy link
Collaborator Author

@pepeiborra pepeiborra Dec 16, 2021

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

lsp-test/test/DummyServer.hs Outdated Show resolved Hide resolved
pepeiborra added a commit to haskell/haskell-language-server that referenced this pull request Dec 18, 2021
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
@pepeiborra
Copy link
Collaborator Author

It would be a good idea to test this change against the HLS test suite before merging.

Testing in haskell/haskell-language-server#2494

pepeiborra added a commit to haskell/haskell-language-server that referenced this pull request Dec 18, 2021
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
pepeiborra added a commit to haskell/haskell-language-server that referenced this pull request Dec 18, 2021
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
pepeiborra added a commit to haskell/haskell-language-server that referenced this pull request Dec 18, 2021
@pepeiborra
Copy link
Collaborator Author

All tests passing now.. I found 2 completions tests in the func-test suite that were using applyEdit and broke with this bug fix. Fixed in the HLS PR.

I think we should merge this before the release

Copy link
Collaborator

@michaelpj michaelpj left a 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.

@michaelpj michaelpj merged commit 6804c53 into master Dec 20, 2021
jneira pushed a commit to jneira/haskell-language-server that referenced this pull request Dec 27, 2021
michaelpj pushed a commit to haskell/haskell-language-server that referenced this pull request Dec 28, 2021
jneira added a commit to haskell/haskell-language-server that referenced this pull request Dec 29, 2021
* 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>
drsooch pushed a commit to drsooch/haskell-language-server that referenced this pull request Dec 29, 2021
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants