search

agronholm / cbor2

Python CBOR (de)serializer with extensive tag support
MIT License
226 stars 59 forks source link

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

Open oxij opened 8 months ago

oxij commented 8 months ago

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.

agronholm commented 7 months ago

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.

theeldermillenial commented 7 months ago

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.

agronholm commented 7 months ago

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.