-
-
Notifications
You must be signed in to change notification settings - Fork 352
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
Get files from Shake VFS from within plugin handlers #4328
base: master
Are you sure you want to change the base?
Conversation
- This avoids a few conversions between Rope and Text in the next commit - Note: Syntactic changes to Development.IDE.Plugin.CodeAction around line 2000 are to work around the following stylish-haskell failure: plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs: <string>:2002:5: error: [GHC-58481] parse error (possibly incorrect indentation or mismatched brackets)
This commit changes plugins to get virtual files from the Shake VFS rather than from the language server's VFS. - Replace `Ide.Types.pluginGetVirtualFile` with `Development.IDE.Core.FileStore.getFileContents` - Replace `Ide.Types.pluginGetVersionedTextDoc` with `Development.IDE.Core.FileStore.getVersionedTextDoc`
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.
LGTM, perhaps @fendor could take a look?
I wonder if we should just run more of our handlers in Action
at the "top-level", it's always a bit awkward running the individual actions repeatedly...
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 idea and execution is good, but could we introduce some plugin util functions that abstract over the most common use cases? I find
liftIO $ runAction "ModuleName.getFileContents" state $ fmap snd $ getFileContents nfp
much less appealing than
lift . pluginGetVirtualFile $ toNormalizedUri uri
In particular
contents <- liftIO $ runAction "Pragmas.GetFileContents" ide $ maybe (pure Nothing) (fmap snd . getFileContents) $ LSP.uriToNormalizedFilePath $ toNormalizedUri uri
looks much more complicated.
Some combinators that make it easier to figure out at a glance what is going on would be great.
I agree with @fendor , it would be great if this is introduced as new util function. And perhaps other plugin can use it. |
@fendor Sorry for the delay! I've added some util functions that make the file accesses look more like they did before this PR |
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.
LGTM, I am now happy with the changes :)
Looks like it has some warnings, which is failing the build. |
Oops, I think I've fixed it now. |
This PR is a step towards #4057.
This PR modifies plugin handlers to get virtual files from the Shake VFS instead of the language server's VFS. Specifically, this PR removes both
pluginGetVirtualFile
andpluginGetVersionedTextDoc
, and replaces them with the (existing)getFileContents
and (new)getVersionedTextDoc
Shake actions.This PR also changes
getFileContents
to return theRope
at hand instead of converting it toText
, which eliminates a few redundant conversions (specifically, at uses ofgetCompletionPrefix[fromRope]
). There are probably more instances in the plugin handlers whereRope
s could replaceText
for more efficiency, but this PR does not address them.