-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
Thanks for the contribution! I haven’t yet tried playing around with this so excuse my ignorance but I’m a bit confused how this relates to the code action that we already have since #81. I thought that already gives you a quickfix button for inserting signatures. Does this only provide a slightly different UX for the same thing or is there something else? |
It's just another interface to the same quick fix. Most of the code is
shared between them, actually.
El sáb., 7 dic. 2019 23:43, Moritz Kiefer <[email protected]>
escribió:
… Thanks for the contribution! I haven’t yet tried playing around with this
so excuse my ignorance but I’m a bit confused how this relates to the code
action that we already have since #81
<#81>. I thought that already
gives you a quickfix button for inserting signatures. Does this only
provide a slightly different UX for the same thing or is there something
else?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#224>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACLQVSVYOT4V6HMARPY7HDQXQRJXANCNFSM4JXRYJYA>
.
|
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.
Looks good to me with some minor comments, thanks a lot!
I’m slightly worried that we’ll end up duplicating all quickfix hints as codelenses at some point because different people prefer different interfaces. Do you have some intuition on which hints make sense as codelenses and which don’t? Personally, I don’t really see why a codelens is a better interface here than a quickfix which seems like it’s exactly meant for this kind of usecase. Given that the change is fairly small and shares most of the code, I’m happy to add it for now either way but I’m curious about how we should make this decision for similar hints.
With a quick fix, you can't actually see what the change is - can you? My
understanding is the code lens shows you a preview of what it is, but quick
fix just says "insert type signature". Maybe there are cases where the
decision is more subjective, and seeing a preview would help?
I haven't used quick fixes yet though, so maybe I'm wrong on their usage.
…On Sun, 8 Dec 2019, 4:06 pm Moritz Kiefer, ***@***.***> wrote:
***@***.**** commented on this pull request.
Looks good to me with some minor comments, thanks a lot!
I’m slightly worried that we’ll end up duplicating all quickfix hints as
codelenses at some point because different people prefer different
interfaces. Do you have some intuition on which hints make sense as
codelenses and which don’t? Personally, I don’t really see why a codelens
is a better interface here than a quickfix which seems like it’s exactly
meant for this kind of usecase. Given that the change is fairly small and
shares most of the code, I’m happy to add it for now either way but I’m
curious about how we should make this decision for similar hints.
------------------------------
In src/Development/IDE/LSP/LanguageServer.hs
<#224 (comment)>:
> @@ -153,6 +158,34 @@ runLanguageServer options userHandlers getIdeState = do
"Exception: " ++ show e
sendFunc $ wrap $ ResponseMessage "2.0" (responseId _id) Nothing $
Just $ ResponseError InternalError (T.pack $ show e) Nothing
+ ResponseAndRequest ***@***.***{_id, _params} wrap wrapNewReq act ->
It would be great if we could deduplicate the request cancellation logic
between ResponseAndRequest and Request.
------------------------------
In src/Development/IDE/LSP/LanguageServer.hs
<#224 (comment)>:
> + -- is in the cancelled set. However, this is unlikely to be a
+ -- bottleneck and the additional check might hide
+ -- issues with async exceptions that need to be fixed.
+ cancelOrRes <- race (waitForCancel _id) $ act lspFuncs ide _params
+ case cancelOrRes of
+ Left () -> do
+ logDebug (ideLogger ide) $ T.pack $
+ "Cancelled request " <> show _id
+ sendFunc $ wrap $ ResponseMessage "2.0" (responseId _id) Nothing $
+ Just $ ResponseError RequestCancelled "" Nothing
+ Right (res, newReq) -> do
+ sendFunc $ wrap $ ResponseMessage "2.0" (responseId _id) (Just res) Nothing
+ case newReq of
+ Nothing -> return ()
+ Just (rm, newReqParams) -> do
+ reqId <- IdInt <$> randomIO
This should use
https://hackage.haskell.org/package/haskell-lsp-0.18.0.0/docs/Language-Haskell-LSP-Core.html#v:getNextReqId
instead of generating a random request id.
------------------------------
In src/Development/IDE/LSP/LanguageServer.hs
<#224 (comment)>:
> @@ -177,11 +210,13 @@ cancelHandler cancelRequest = PartialHandlers $ \_ x -> return x
-- and defer precise processing until later (allows us to keep at a higher level of abstraction slightly longer)
data Message
= forall m req resp . (Show m, Show req) => Response (RequestMessage m req resp) (ResponseMessage resp -> FromServerMessage) (LSP.LspFuncs () -> IdeState -> req -> IO resp)
+ | forall m rm req resp newReqParams newReqBody . (Show m, Show rm, Show req) => ResponseAndRequest (RequestMessage m req resp) (ResponseMessage resp -> FromServerMessage) (RequestMessage rm newReqParams newReqBody -> FromServerMessage) (LSP.LspFuncs () -> IdeState -> req -> IO (resp, Maybe (rm, newReqParams)))
It would be great if you could add a comment that explains the usecase
here. i.e., an executeCommandHandler that generates an applyWorkspaceEdit
request as the response.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#224>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAFDDVFRQLU6GV2SQCOC53QXULRNANCNFSM4JXRYJYA>
.
|
My view is that this change is really about surfacing information to the user about the top level signature. It's also a way to insert it back into the program, but that's a minor part. As such, it's quite different from other fixes. |
@ocharles The title of the quickfix includes the signature as well, see https://github.com/digital-asset/ghcide/blob/5091a1d2021f89f2b4209d498144b061c1ea7e95/src/Development/IDE/LSP/CodeAction.hs#L150 @ndmitchell Yeah although as mentioned above both the quickfix and the codelens surface the information. It’s a bit more visible with the codelens but that’s fairly specific to how VSCode displays codelenses and quickfixes. There is nothing stopping an editor from displaying quickfixes inline. |
One possibility is to remove "add signature" from the code lens (but not from the quick fix). That way it looks like the missing type signature had been "added" to the source code, sort of what @ndmitchell said above (in fact, that was my original goal, it just turned out that code lenses require a command, which led naturally to adding the signature when clicked). @cocreature my opinion is that no more quick fixes should be turned into code lenses. However, I think the way they are designed is a perfect fit for missing type signatures, since we can show the signature exactly where you would write it on code (at least in VS Code), and it's some sort of "comment" over your code. There's more room, however, to show some additional information about the code as lenses, but that's the topic of another issue. |
@cocreature I have implemented your suggestions in the latest commit. I have also changed the UI for the code lens, so that it only shows the missing type signature, and not the "add signature" text. That makes it look more like a "comment" over the code, rather than a quick fix. |
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.
Nice, thank you!
* Code lens for missing signatures * Fix tests * Implement suggestions by @cocreature
* Code lens for missing signatures * Fix tests * Implement suggestions by @cocreature
* Code lens for missing signatures * Fix tests * Implement suggestions by @cocreature
This PR adds code lenses for missing type signatures, as discussed in #88. Missing type signatures appear as comments on top of the start of the definition, as shown in this image. When the user clicks on the definition, it's added to the source code.
Unfortunately, the specification of code lenses is not as straightforward as code actions. Instead of returning how to edit the file directly, we need to return an identifier with a command, which is then called when the user clicks on the code lens. That command does not return what to do either, instead it may request things from the client by using a server request. Hence the need for a new type of response in
WithMessage
, with both the response to the request and an optional user request. This new constructor might be merged withwithResponse
, but I didn't take that step; let me know if you prefer me to do so.