Expose *_encoders and use them to replace canonical param of CBOREncoder#227
Expose *_encoders and use them to replace canonical param of CBOREncoder#227oxij wants to merge 1 commit intoagronholm:masterfrom
*_encoders and use them to replace canonical param of CBOREncoder#227Conversation
…REncoder` This changes the API a little, yes.
|
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. |
|
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. |
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. |
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_containerthere, 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
cbor2in pwebarc, which would be annoying.