feat: support for CBOR #32
Conversation
There was a problem hiding this comment.
Besides the comments, I strongly believe we need to deal with the nim-serde pragmas in some way, even if they're not supported. A few options:
- issue an error (preferably a compilation error, if possible) if you try to use an annotated type with CBOR. This is what I prefer. Safest, least likely to cause surprise;
- add documentation describing the behavior. Better than nothing, can still cause surprise, at least is written somewhere.
Either way, the documentation needs to be updated. I'm sure @emizzle will have better suggestions than mine as to how to make this integration as seamless as possible.
| return failure(e.msg) | ||
| except OSError as e: | ||
| return failure(e.msg) | ||
|
|
|
|
||
| ?c.next() | ||
| return success(val) | ||
|
|
serde/cbor/deserializer.nim
Outdated
| return failure(newCborError("Expected positive integer, got " & $c.kind)) | ||
| let val = c.intVal.BiggestUInt | ||
|
|
||
| ?c.next() |
There was a problem hiding this comment.
I'm honestly not very fond of this. Took me a bit to figure out what this was doing, and I don't think I've seen this being used like that elsewhere.
There was a problem hiding this comment.
serde/cbor/deserializer.nim
Outdated
| let n = c.s.readData(buf[0].addr, buf.len) | ||
| if n != buf.len: | ||
| return failure(newCborError("truncated read of CBOR data")) | ||
| tryNext(c) |
There was a problem hiding this comment.
Yeah... I don't like those things that look like proc calls syntactically but then stuff a return into the code. It makes things very hard to read. It makes things look more like exception-based code (i.e. a tad cleaner), but it's surprising behavior when using result types IMHO.
There was a problem hiding this comment.
If you really want to do something like that, it has to at least look like an explicit return IMHO. Something like:
returnOnFail c.next()and then have returnOnFail as your template.
There was a problem hiding this comment.
I have removed this template and replaced with ? template.
serde/cbor/deserializer.nim
Outdated
|
|
||
| func arrayLen*(c: CborParser): int = | ||
| ## Return the length of the array that the parser is positioned on. | ||
| assert(c.kind == CborEventKind.cborArray, $c.kind) |
There was a problem hiding this comment.
Why did you decide to throw an exception here instead of using result as you did with bytesLen? Also, I don't see this proc being used anywhere (nor bytestLen)?
serde/cbor/jsonhook.nim
Outdated
| import ./errors | ||
| import ./deserializer | ||
|
|
||
| proc toJsonHook*(n: CborNode): JsonNode = |
There was a problem hiding this comment.
I'd call this toJson and keep it uniform with nim-serde API.
serde/cbor/serializer.nim
Outdated
| except Exception as e: | ||
| return failure(e.msg) | ||
|
|
||
| proc writeCborHook*(str: Stream; dt: DateTime): ?!void = |
There was a problem hiding this comment.
Would probably rename all of those to toCbor.
| func keysNotIn[T](json: JsonNode, obj: T): HashSet[string] = | ||
| let jsonKeys = json.keys.toSeq.toHashSet | ||
| let objKeys = obj.fieldKeys.toHashSet | ||
| difference(jsonKeys, objKeys) |
There was a problem hiding this comment.
I wouldn't move those around for no reason. It's actually confusing cause I thought you were using them in the CBOR code, but then later realized you're not.
There was a problem hiding this comment.
@gmega I've moved this to serde/json/helper.nim to separate helper functions from the core logic. This helps improve the overall structure and maintainability of the code
| check roundtrip.coordinates.x == original.coordinates.x | ||
| check roundtrip.coordinates.y == original.coordinates.y | ||
| check roundtrip.coordinates.label == original.coordinates.label | ||
|
|
| import pkg/serde/cbor | ||
| import pkg/questionable | ||
| import pkg/questionable/results | ||
|
|
This PR introduces support for canonical CBOR serialization in compliance with RFC 8949.
Note that this implementation does not yet support serde features such as
OptIn, OptOut, Strict, or serialize/deserializepragma annotations