Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oxij
Copy link

@oxij oxij commented Mar 21, 2024

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.

…REncoder`

This changes the API a little, yes.
@agronholm
Copy link
Owner

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
Copy link

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-expose default_encoders and allow changing them in CBOREncoder again
3 participants