search

quininer / cbor4ii

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

Remove `*Start` types from dec module #24

Closed vmx closed 1 year ago

vmx commented 2 years ago

Instead of having TagStart, ArrayStart and MapStart, implement functions directly on the concrete types.

This PR tries to address the comment at https://github.com/quininer/cbor4ii/pull/22#issuecomment-1108791827.

I've based it on the 0.3 branch. But this also means that I haven't tested it with my own Serde implementation (which is still on on 0.2.x), but I don't see a reason why it shouldn't work there. I didn't want to spent too much time on this PR as I'm not sure if that's a good approach or not. Though I'm happy to spend more time on it, in case it's the right direction.

quininer commented 2 years ago

This looks good and I kind of like the design. maybe the *Start type from enc.rs can be integrated as well?

vmx commented 2 years ago

Sounds like I'm on the right track, I'll look into enc.rs next.

quininer commented 1 year ago

If you are interested in resolving the conflict, I will merge it.

vmx commented 1 year ago

Rebased. I'll now look into enc.rs as promised (sorry for not looking into this earlier)

vmx commented 1 year ago

I've pushed the encoding part to this branch, if you'd like it in a separate PR, please let me know.

I first wasn't sure if it makes sense on the encoding, but I like the outcome. It makes the code more consistent I think and also less public exports are needed.

For strings I've implemented it in BadStr as it already existed. I thought about introducing a new String type, but that felt a bit wrong to introducing a new type, just for indefinite length strings.

And of course I'd like to thank you for pinging me before doing a breaking release.

vmx commented 1 year ago

I guess I had those value/values functions public for a reason, but I can't recall it. Hence I've just removed them, which makes the code cleaner.

quininer commented 1 year ago

Thank you!