cbor2 icon indicating copy to clipboard operation
cbor2 copied to clipboard

Expose `*_encoders` and use them to replace `canonical` param of `CBOREncoder`

Open oxij opened this issue 11 months ago • 3 comments

This changes the API a little, but makes many things much simpler. Fixes #226.

I also did some more changes to make the C module compile and all the bundled unit tests pass, see there. But fixing the C module actually makes things worse in practice, because the Python module has some other nice API changes that have to be either re-implemented in C now (see my attempt at implementing CBOREncoder_encode_container there, but there's more API missing) or, alternatively, the design of the C module needs to change. Personally I would prefer the second option: IMHO, it should only implement the low-level encoders and leave the rest to the Python code.

Feel free to take and edit these changes however you like, I'd agree to any other alternative solution to #226, because otherwise I'd have to vendor cbor2 in pwebarc, which would be annoying.

oxij avatar Mar 21 '24 16:03 oxij

There's a similar PR (#225) ongoing for decoders, so I'm willing to consider this. I have precious little bandwidth for this project though, so the test failures should be fixed at least.

agronholm avatar Apr 10 '24 22:04 agronholm

Hey, I authored #227

I like your idea. I don't see a need to have a complete feature parity C-extension. Ideally the C-extension only implements the things that benefit compiled code, but obviously this is up to @agronholm

Maybe to make things more consistent and save the repo maintainer some headaches, we could merge efforts between my PR and yours? They are functionally trying to do the same thing, just for encoders vs decoders.

theeldermillenial avatar Apr 11 '24 00:04 theeldermillenial

Maybe to make things more consistent and save the repo maintainer some headaches, we could merge efforts between my PR and yours? They are functionally trying to do the same thing, just for encoders vs decoders.

Totally. The C extension was added during a time I had delegated maintainership authority to another person. I'm all for narrowing down the C extension strictly to what benefits performance.

agronholm avatar Apr 11 '24 06:04 agronholm