Replace play-json with circe #165
Conversation
- fix $/cancelParams - remove scala-json-rpc - remove dead code
| import langserver.messages.WorkspaceExecuteCommandRequest | ||
| import langserver.messages.WorkspaceSymbolRequest | ||
| import langserver.messages.WorkspaceSymbolResult | ||
| import langserver.messages._ |
| // case Right(conf) => | ||
| // logger.info(s"Configuration updated $conf") | ||
| // configurationSubscriber.onNext(conf) | ||
| // } |
|
|
||
| case event => | ||
| logger.warn(s"Unhandled file event: $event") | ||
| () |
There was a problem hiding this comment.
Doesn’t logger.warn already return unit?
There was a problem hiding this comment.
I think so, but you better add this comment to @olafurpg's PR. This is just based on it.
| | "signatures" : [ | ||
| | { | ||
| | "label" : "<init>(name: String)User", | ||
| | "documentation" : null, |
There was a problem hiding this comment.
Any concern that the previous json library did not serialization null values and circe appears presently configured to serialization them as JsNull?
There was a problem hiding this comment.
From a protocol POV it should be irrelevant.
As mentioned in #165 (comment) I treated this as an implementation detail. Changing the default pretty-printer would've been much more bothersome.
There was a problem hiding this comment.
Printer is easy to change, IIRC it’s just printer.copy(dropNullValues = true)
I don’t think changing json libraries should require test changes. Are we comparing raw strings where we should be comparing parsed json in tests?
There was a problem hiding this comment.
Printer is easy to change, IIRC it’s just printer.copy(dropNullValues = true)
It is, but then you have to pass around the printer instance and do json.pretty(printer) instead of json.spaces2.
Are we comparing raw strings where we should be comparing parsed json in test
Using directly the json strings was a choice we took a while ago and it served us well so far.
I would be ok in comparing with parsed json instead. Something like:
import io.circe.Json.{obj, arr, fromInt => int, fromString => str, False}
// ...
check(
"foo",
obj(
"isIncomplete" -> False,
"items" -> arr(
obj(
"label" -> str(label),
"kind" -> int(kind.value),
"detail" -> str(detail),
"sortText" -> str("00000")
)
)
)
)wdyt @olafurpg?
There was a problem hiding this comment.
What do we gain by moving to comparing parsed json instead of strings? I personally think strings are nice because
- test errors are reported as unified diff which are easy to read
- false negatives are easy to fix by copy-pasting the failed test output and pasting it into the field
There was a problem hiding this comment.
For the tests I would recommend using dropNullValues = true to minimize the string output, I have no preference for the messages sent from MessageWriter
There was a problem hiding this comment.
I don’t think changing json libraries should require test changes.
I think it's totally fine if the pretty-printers are incompatible. It would be nice if there was a compatible printer so we can keep the tests unchanged, I personally liked the play-json pretty-printer output since it was quite compact compared to the output in these tests.
We've started using cats, but so far it was a sub-dependency of circe and monix.
This is based on #164.
There's some bits I'm not particularly happy with, but not too bad!
The only relevant commit is d9b38bc