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

Correctly pass VersionedTextDocumentIdentifier through hls #3643

Merged
merged 7 commits into from
Jun 11, 2023

Conversation

maralorn
Copy link
Contributor

This is an attempt at a continuation of #3566 because I don’t have rights to push to that branch.

Sadly, while I can’t reproduce the concrete error message we are trying to fix here. There are still a lot of files in which hlint fixes don’t get applied for me.

@maralorn
Copy link
Contributor Author

Okay, I can confirm, that the failing edits happen in files with CPP and the error log clearly shows that it is because of the CPP.

@maralorn
Copy link
Contributor Author

I still need to fix compilation with wingman.

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.

Looks great!

getFileEdit = flip $ getSrcEdit state . replaceRefs newName
fileEdits <- mapM (uncurry getFileEdit) filesRefs
getFileEdit (uri, locations) = do
verTxtDocId <- lift $ getVersionedTextDoc (TextDocumentIdentifier uri)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this may well fix the rename plugin to work properly if one of the files that we rename in has been changed in the editor!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had the feeling that I am fixing a bug here.

@@ -124,6 +124,7 @@ data FileContext = FileContext
, fc_range :: Maybe (Tracked 'Current Range)
-- ^ For code actions, this is 'Just'. For code lenses, you'll get
-- a 'Nothing' in the request, and a 'Just' in the response.
, textVersion :: TextDocumentVersion
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like there's a URI field too, so can we just make it a VTDI?

@@ -622,12 +622,13 @@ mkWorkspaceEdits
:: DynFlags
-> ClientCapabilities
-> Uri
-> TextDocumentVersion
Copy link
Collaborator

Choose a reason for hiding this comment

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

VTDI?

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jun 11, 2023
@maralorn maralorn force-pushed the with-version-identifier branch from 5b38d24 to b6f4c66 Compare June 11, 2023 15:01
@maralorn
Copy link
Contributor Author

I think there was a transient test error on windows. Can we maybe restart it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants