quininer / cbor4ii

CBOR: Concise Binary Object Representation
MIT License
54 stars 5 forks source link

Making more things public #16

Closed vmx closed 2 years ago

vmx commented 2 years ago

I've now created a working Serde implementation for my needs, that is based on cbor4ii core. Though I still need to patch core as some things that I need for the deserializer are not public.

core::marker, core::dec::peek_one, core::dec::pull_one and core::dec::decode_len are currently pub(crate), but are needed by my deserializer (which is more or less a copy of yours). Could those be made public?

Besides those, I've copy and paste also other things. I'm OK with having those duplicated if you don't think they should be part of the public interface, though I surely prefer having less code in my crate. Those are:

quininer commented 2 years ago

ScopeGuard is very small and explicit code, I think copying it is fine. IoReader and IoWriter fit into utils, just like SliceReader and BufWriter.

peek_one these functions I'd rather keep private as I don't think the current api is the best way to expose them.

vmx commented 2 years ago

ScopeGuard is very small and explicit code, I think copying it is fine.

:+1:

IoReader and IoWriter fit into utils, just like SliceReader and BufWriter.

Cool, I'll create a PR.

peek_one these functions I'd rather keep private as I don't think the current api is the best way to expose them.

Those are the important ones. Without those exposed, I need to keep a fork of the core, just to be able to use those in my deserializer.

quininer commented 2 years ago

I'm reluctant to expose them prematurely and make promises of stability.

marker, peek_one, pull_one are also small and explicit, I don't think copying them is a problem. I don't think you need decode_len, you should be able to use ArrayStart/MapStart instead.

vmx commented 2 years ago

marker, peek_one, pull_one are also small and explicit, I don't think copying them is a problem.

They are indeed small and stand-alone, I should've checked that. Thanks for mentioning.

I don't think you need decode_len, you should be able to use ArrayStart/MapStart instead.

That's a good one. I've opened https://github.com/quininer/cbor4ii/pull/18 that also applies that change to your deserializer.

The PR to move IoReader and IoWriter (renamed from IoWrite) into core::utils is at https://github.com/quininer/cbor4ii/pull/17.

vmx commented 2 years ago

Thanks again for all the feedback. May I kindly ask for a release? I think I'm all set and my Serde implementation works as expected.

quininer commented 2 years ago

I just released 0.2.11 :)