replace this package with https://github.com/beacon-biosignals/MsgPack2.jl #30
Conversation
…ture parity with original MsgPack.jl (#17)
|
Awesome, looks all green. I'll wait for an approval here, then I'll merge and tag a v1.0.0 release (major version bump since it's quite a big code change, though I made sure to leave in deprecation paths where they seemed needed). |
Project.toml
Outdated
| version = "0.2.0" | ||
| uuid = "f3bf5812-ae52-11e9-182b-097c550858cf" | ||
| authors = ["Jarrett Revels <jarrettrevels@gmail.com>"] | ||
| version = "0.1.0" |
There was a problem hiding this comment.
Version needs to be bumped here.
src/pack.jl
Outdated
| """ | ||
| function pack(io::IO, x) | ||
| pack_type(io, msgpack_type(typeof(x)), x) | ||
| return io |
There was a problem hiding this comment.
I'm curious if it's useful to return io here, or if nothing might be better?
I guess io would be better if packing things in a pipeline, but I'm not sure how common that is (or even if it's uncommon, if it's something we want to support).
There was a problem hiding this comment.
nothing seems like a sensible return value here so I've gone ahead and made that change
| error("no non-`AnyType` MsgPack mapping found for ", typeof(x), "; please ", | ||
| "overload `msgpack_type` for this type.") | ||
| end | ||
| return pack_type(io, tx, x) |
There was a problem hiding this comment.
In some cases, pack_type returns the result from write() (an Int), while in other cases, the return value is nothing.
The same seems to be true for pack_format.
Since the return value doesn't seem to be used in any calls, this may not matter, but maybe it's worth always returning nothing?
Alternatively, if returning the result of write (the number of byte written), then some functions would need to accumulate those values.
There was a problem hiding this comment.
Yup, good point - the return values of these functions (which are just meant to be internal) should probably be nothing. Just would require a bit of churn to refactor; I'll open a follow-on issue.
There was a problem hiding this comment.
That would be fine. How about just creating our own write function? Something like
function write(args...)
Base.write(args...)
nothing
end(Probably simple enough that @inline isn't necessary?)
src/unpack.jl
Outdated
| elseif byte >= magic_byte_min(IntFixNegativeFormat) | ||
| return unpack_format(io, IntFixNegativeFormat(reinterpret(Int8, byte)), T) | ||
| else | ||
| invalid_unpack(io, byte, AnyType(), T) |
There was a problem hiding this comment.
Unreachable code? I guess it's good to keep it here to guard against developer error... ;-)
src/unpack.jl
Outdated
| elseif byte >= magic_byte_min(IntFixNegativeFormat) | ||
| return unpack_format(io, IntFixNegativeFormat(reinterpret(Int8, byte)), T) | ||
| else | ||
| invalid_unpack(io, byte, t, T) |
| elseif byte === magic_byte(Bin32Format) | ||
| return unpack_format(io, Bin32Format(), T) | ||
| elseif byte >= magic_byte_min(IntFixNegativeFormat) | ||
| return unpack_format(io, IntFixNegativeFormat(reinterpret(Int8, byte)), T) |
There was a problem hiding this comment.
When I see these long lists, I really want a switch or match statement. (Of course, those exist in packages, and it's nice not to depend on unnecessary packages...)
Let's see how CI runs correctly here; seems like codecov/Travis are already set up so should work in theory