Type parsing more strongly #29
Conversation
phoebe-lew
requested review from
mistermoe,
jiyoonie9,
michaelneale,
KendallWeihe and
ethan-tbd
as code owners
| @@ -0,0 +1,35 @@ | |||
| package protocol.models | |||
There was a problem hiding this comment.
Within our tests we have packages...
modelsprotocolprotocol.models
We should reconcile
There was a problem hiding this comment.
Didn't you have a PR open to fix that?
| @@ -0,0 +1,24 @@ | |||
| package models | |||
|
|
|||
| import typeid.TypeID | |||
There was a problem hiding this comment.
Just as an aside, when we first introduced the typeid-js dependency in tbdex-js we married ourselves to that particular package, which may or may not have multi-language support. In this case it appears it does have multi-language support. But, just a forewarning on rubber stamping new dependencies.
There was a problem hiding this comment.
I would suggest NOT rolling our own token lib and implementing typeid in languages where it's currently not supported.
|
|
||
| private fun verify() { | ||
| validate() | ||
| sealed class Message { |
There was a problem hiding this comment.
Should this belong in the web5 repo? Seems related to DWNs, and not specifically to tbdex.
There was a problem hiding this comment.
@andresuribe87 this is actually a tbdex message as described in the tbdex protocol here
There was a problem hiding this comment.
Thanks @ethan-tbd - is that the right place long term for it?
There was a problem hiding this comment.
Yeah, tbdex has two overarching data structures: resources and messages. The latter are static/immutable data structures and the latter are ephemeral chatty & context dependent structures. I recognize DWNs offer something similar but even in such a world the tbdex application (built on top of the platform primitives) would introduce these concepts. Coalescing the two message concepts is ill advised IMO if only because they are semantically distinct matters.
protocol/src/main/kotlin/Mapper.kt
Outdated
| } | ||
|
|
||
| // TODO make private and add method for calling readTree | ||
| val objectMapper: ObjectMapper = ObjectMapper() |
There was a problem hiding this comment.
Curious why you went with jackson for json serialization vs kotlinx serialization?
There was a problem hiding this comment.
I think it's just what we've been using. Is there an advantage to using kotlinx instead?
There was a problem hiding this comment.
The two advantages I know of:
- Speed
- Verbosity
I don't know how well it's supported in different versions of kotlin though, or different platforms.
There was a problem hiding this comment.
Relevant comment #31 (comment)
There was a problem hiding this comment.
@andresuribe87 , this was a grandfathered decision from @bradleydwyer and @tomdaffurn. I personally wanted to be able to use kotlinx, but also heard from @alecthomas that it wasn't ready for prime time. Kotlinx has a MUCH nicer interface.
There was a problem hiding this comment.
Yeah, I tried and failed to use kotlinx serialisation. I really liked the API too, but IIRC it was just generally quite restrictive and fiddly to use.
closes #9 , #13 and refactors types