sbt / sbt-remote-control

Create and manage sbt process using unicorns and forks
Other
74 stars 14 forks source link

Wip/fix serialized value #246

Closed jsuereth closed 9 years ago

jsuereth commented 9 years ago

Review by @havocp / @eed3si9n

Note: this is but one minor step in the process of getting to a better Serialized Value. Primary outcomes here:

  1. Avoid serializing/parsing json strings like 30 times in their lifecycles
    • For this we altered the JSONPickle class directly
  2. Expose what we need in SerializedValue and try to hide JsonValue (not succesful, but a start)
  3. Remove the need for json4s native parser. Instead we use Jawn and a hand-written toString implementation for now. This can help avoid the re-ordering issues we saw before.

Again this is just a step in the right direction, not a full cleanup. However, I'd like to move from working state to working state, hence the intermediate PR.

havocp commented 9 years ago

Looks like a good improvement, LGTM.

Looking forward -

The separation between SerializedValue and JsonValue was based on the thought that sbt internals might use JsonValue and plugins/activator wouldn't need to know about JSON at all. But when I ported Activator that turned out to be pretty false, Activator needs JSON. Also I think we use JSON heavily in our test suite and plugin test suites also would need it.

Where I think we might end up later:

Just a first draft idea.

jsuereth commented 9 years ago

I like the suggestions. A good path to walk down, IMHO.

I do like consolidating on SerialziedValue + SPickler/Unpickler for defingin protocols, while we hide details of JSON/Binary formats behind it. as @eed3si9n would be found of, we can avoid defining binary until we have pragmatic evidence it's needed.

jsuereth commented 9 years ago

@havocp / @eed3si9n ready for a second review if you want.