-
Notifications
You must be signed in to change notification settings - Fork 92
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
Avoid couple partial functions in lsp-test #546
Conversation
@@ -513,7 +513,7 @@ logMsg t msg = do | |||
shouldColor <- asks $ logColor . config | |||
liftIO $ when shouldLog $ do | |||
when shouldColor $ setSGR [SetColor Foreground Dull color] | |||
putStrLn $ arrow ++ showPretty msg | |||
B.putStrLn $ arrow <> encodePretty msg |
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.
It seems pointless to convert pretty-printed json bytestring to String just to print it.
Wouldn't directly printing ByteString be less resource intensive?
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.
I have no idea, but seems sensible
@@ -421,7 +421,7 @@ sendRequest method params = do | |||
reqMap <- requestMap <$> ask | |||
liftIO $ | |||
modifyMVar_ reqMap $ | |||
\r -> return $ fromJust $ updateRequestMap r id method | |||
\r -> return $ fromMaybe r $ updateRequestMap r id method |
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.
updateRequestMap returns Nothing when request with given LspId is already in it.
I don't see how it makes sense to crash in such cases as opposed to returning the origial map unmodified. Similarly in the file below.
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.
It's quite bad: that means we've somehow reused an ID which means stuff will break. So I think this probably should throw, but maybe not by using fromJust
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.
Alright I'll play around with it locally to see if I can reproduce the me play around with how this works to see if I can reproduce the Timed out waiting to receive a message from the server.
locally and then get back to this or close this.
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.
Thanks! I do think it's a good idea to blow up somehow if we have reused ids, though.
Found this which answers my questions about test flakiness so closing this PR. |
Was anyone able to figure out why there are sometimes timeout messages (apparently unrelated to actual code changes) of the following king in hls test suite?
U briefly checked how lsp-test works and was surprised by the amount of partial functions used.
Here's a few easy to replace occurrences of fromJust + couple hard-to-resist hlint suggestions.