Further hlint resolve changes. #3685
Conversation
hls-plugin-api/src/Ide/Types.hs
Outdated
| @@ -1069,8 +1075,17 @@ mkCodeActionWithResolveAndCommand plId codeActionMethod codeResolveMethod = | |||
| -- resolve data type to allow the server to know who to send the resolve request to | |||
| supportsCodeActionResolve caps -> | |||
| pure $ InL (wrapCodeActionResolveData pid <$> ls) | |||
| -- If they do not we will drop the data field, in addition we will populate the command | |||
| -- field with our command to execute the resolve, with the whole code action as it's argument. | |||
| {- Note [Code action resolve fallback to commands] | |||
There was a problem hiding this comment.
the note convention is generally to put these at the top level (so they're easier to read) and at the end of the file
hls-plugin-api/src/Ide/Types.hs
Outdated
| -- pas it to the resolve handler (which expects a whole code action) | ||
| -- pass it to the resolve handler (which expects a whole code action) | ||
| -- It should be noted that mkLspCommand already specifies the command | ||
| -- to the plugin, so we don't need to do that here. |
There was a problem hiding this comment.
| -- to the plugin, so we don't need to do that here. | |
| -- to the plugin, so we don't need to do that here and can just use the same command name |
hls-plugin-api/src/Ide/Types.hs
Outdated
| , ca ^. L.command == _command | ||
| , ca ^. L.isPreferred == _isPreferred | ||
| , ca ^. L.data_ == _data_ | ||
| , ca ^. L.disabled == _disabled -> do |
There was a problem hiding this comment.
lol, this is kind of annoying. I guess there isn't a super easy way to check if two records are the same except for a specific field. One thing we could do is define a helper in lsp-types or similar:
checkCodeActionResolveFields :: [Text] -> CodeAction -> CodeAction -> Maybe Text
i.e. something that takes the set of client-allowed updateable fields, the two code actions, and either passes or gives you a helpful error. Then that's just exactly the thing everyone will need.
hls-plugin-api/src/Ide/Types.hs
Outdated
| , ca ^. L.disabled == _disabled -> do | ||
| _ <- sendRequest SMethod_WorkspaceApplyEdit (ApplyWorkspaceEditParams Nothing wedits) (\_ -> pure ()) | ||
| pure $ Right Data.Aeson.Null | ||
| | otherwise -> pure $ Left $ responseError "CodeAction fields beside edit changed during resolve" |
There was a problem hiding this comment.
e.g. it would be nice to say which one! And maybe even include the two code actions. This is all very much bonus points and it all seems pointless... until someone reports a bug with one of the errors, and then you're always grateful to have the detail!
| CodeAction {_data_ = Just (fromJSON @HlintResolveCommands -> (Error (T.pack -> str)))} = | ||
| pure $ Left $ ResponseError (InR ErrorCodes_ParseError) str Nothing | ||
| resolveProvider _ _ _ CodeAction {_data_ = _} = | ||
| pure $ Left $ ResponseError (InR ErrorCodes_InvalidParams) "Unexpected argument for code action resolve handler: (Probably Nothing)" Nothing |
There was a problem hiding this comment.
same comment as the other PR, throw the Value in the error
| edit <- ExceptT $ liftIO $ applyHint recorder ideState file (Just oneHint) verTxtDocId | ||
| resolveProvider recorder ideState _ | ||
| ca@CodeAction {_data_ = Just (fromJSON -> (Success (ApplyHint verTxtDocId oneHint)))} = pluginResponse $ do | ||
| file <- getNormalizedFilePath (verTxtDocId ^. LSP.uri) |
There was a problem hiding this comment.
again, for minor deduplication, I'd be tempted to do just the pattern match on the HlintResolveCommands later, i.e.
... fromJson -> command) = pluginResponse $ do
file <- getNormalizedFilePath ...
edit <- case command of ...
pure $ ca & LSP.edit ?~ edit
This stuff is a bit nitpicky but I think it's usually worth it
There was a problem hiding this comment.
The reason why we can't is because we need the uri to do anything useful, and that's encoded in the data. I'm gonna try and see if I can model resolve provider infrastructure on what we have for commands. That way hls core can do the json decoding and also store a uri to pass on to the handlers for us to use right away.
There was a problem hiding this comment.
Ah right, I see. Yeah, maybe that's natural. But then again maybe we should leave this stuff up to plugins. Something like the URI you definitely always need though.
I missed some of michealpj's pr suggestions from last pr, so this pr addresses them.