Support encoding indefinite containers #256

CZDanol · 2025-03-26T12:52:14Z

Changes

Added the indefinite_containers parameter to the encoder functions. If the parameter is set to True, containers (maps and arrays) are encoded an indefinite containers.

I am using this library to for unittesting a C++ CBOR generator, for which case I needed to generate structures with indefinite containers to be able to do 1:1 byte checks.

agronholm · 2025-03-26T13:57:34Z

How are the tests passing for you locally?

CZDanol · 2025-03-26T14:01:11Z

I'm getting all sorts of SKIPPED [1] tests/test_types.py:97: requires CPython and I don't know how to resolve it :( So I am only managing to test the python implementation locally.

agronholm · 2025-03-26T14:04:29Z

I'm getting all sorts of SKIPPED [1] tests/test_types.py:97: requires CPython and I don't know how to resolve it :( So I am only managing to test the python implementation locally.

This message indicates that the C extension is unavailable. So likely it didn't compile successfully, or you didn't do it in the first place. How are you running the tests? tox is a good way to test against multiple Python versions and its configuration on cbor2 ensures that the C extensions get built.

CZDanol · 2025-03-26T14:08:35Z

I was just running pytest on python3.12.
I now tried tox, it gives me "OK" even when I have garbage in the .c files.

CZDanol · 2025-03-26T14:10:30Z

WOOHOO! Passes the tests now :)

agronholm · 2025-03-26T14:10:31Z

That's odd. Try setting CBOR2_BUILD_C_EXTENSION=1 in your environment and then run pip install -e .. That should make sure that the compilation happens.

coveralls · 2025-03-26T14:12:11Z

coverage: 93.607% (+0.07%) from 93.542%
when pulling 1aa0f99 on CZDanol:indefinite-containers
into d9cee77 on agronholm:master.

CZDanol · 2025-03-26T14:12:46Z

That CBOR2_BUILD_C_EXTENSION=1 pip install -e . seems to have made the trick. Thanks :)

docs/usage.rst

docs/versionhistory.rst

agronholm · 2025-03-27T16:01:25Z

From a cursory look, only minor code style issues remain. In Python code, I prefer a blank line after a control block if the code continues on the same level. In C code, I prefer a space between if and the opening parentheses. I'll give this a proper review some time later.

CZDanol · 2025-03-28T10:50:33Z

No problem with doing these changes, but wouldn't these styling issues be better resolved by a pre-commit formatter hook?

agronholm · 2025-03-28T11:28:54Z

If you can point me to a Ruff rule (or a rule in another linter) that can enforce these, I would be most grateful!

CZDanol · 2025-03-28T14:44:34Z

Don't know about the Python part, but I am quite positive there is an option for the ifs in ClangFormat. There seems to be no autoformatting for the C code set up at the moment.

agronholm · 2025-03-31T09:41:30Z

Something occurred to me: couldn't we simply wrap indefinite containers to mark them to be encoded as such?

CZDanol · 2025-03-31T09:45:45Z

You mean that you would need to use a specific class other than the dict? I suppose that would also work, but it wouldn't be that practical for my use case - I need to encode all containers as indefinite, implicitly. I could imagine both approaches being accepted/implemented - the wrapping approach would offer granular control over how a specific element gets encoded, whereas my PR provides a general configuration for the encoder, which I believe is also valid and useful.

agronholm · 2025-03-31T09:47:51Z

Alright, I'll take that under advisement. Reviewing this is still on my to-do list, but I don't expect any trouble from that.

source/encoder.c

CZDanol · 2025-04-20T16:34:14Z

source/encoder.c


 // CBOREncoder.encode_length(self, major_tag, length)
 static PyObject *
 CBOREncoder_encode_length(CBOREncoderObject *self, PyObject *args)
 {
    uint8_t major_tag;
-    uint64_t length;
+    uint64_t length = -1;


Yes, -1 becomes a very large integer.

I did this because I was too lazy to set up a custom structure for the uint64_or_none that would be required for PyArg_ParseTuple. It would be cleaner to have it distinct, yes. It is somewhat dirty, so I didn't want to bring this approach to other places.

Should I remake it?

Well, I just thought that if using -1 was acceptable, why not just use it directly in encode_length()?

I guess I can - if you accept that solution, I can make it that way.

Do you see any downsides when compared to the current code in this PR?

At the current state of the code, probably not.

CZDanol · 2025-05-13T11:27:27Z

Hello, no updates from you for quite some time

agronholm · 2025-05-13T11:29:23Z

Ah...sorry, I've taken a look at this a few times but my attention span wasn't quite long enough to determine what I want here. I'll reply in the ongoing convo.

CZDanol · 2025-05-15T08:51:40Z

Okay, so apparently in the tests you are trying to encode the maximum length (u64-1) on a few places, which is now producing invalid results - so we need that discernation. I will revert the changes (and make the uint64_t_or_none proper)

agronholm · 2025-05-15T08:56:36Z

Okay, so apparently in the tests you are trying to encode the maximum length (u64-1) on a few places, which is now producing invalid results - so we need that discernation. I will revert the changes (and make the uint64_t_or_none proper)

I think that'd be for the best. I sometimes lose sleep over details like this :)

Add the indefinite_containers parameter to the encoder functions. If the parameter is set to True, containers (maps and arrays) are encoded an indefinite containers.

CZDanol · 2025-05-15T09:00:25Z

All right, done. Sorry about the mess. I have squashed things up because I felt like it's way too much.

(Also I was too lazy to remember how to run tests on my local machine, but I've done that now as well)

agronholm · 2025-06-05T09:29:18Z

I'm too attention deficient to wrangle any more about this, so I just merged it.

CZDanol · 2025-06-05T10:03:12Z

Okay, thanks :D

CZDanol force-pushed the indefinite-containers branch 9 times, most recently from b473365 to b672722 Compare March 26, 2025 13:55

CZDanol force-pushed the indefinite-containers branch from b672722 to 088d68b Compare March 26, 2025 14:00

CZDanol force-pushed the indefinite-containers branch from 088d68b to 597fc86 Compare March 26, 2025 14:01

CZDanol force-pushed the indefinite-containers branch from 597fc86 to b8b0d89 Compare March 26, 2025 14:09

agronholm reviewed Mar 27, 2025

View reviewed changes

docs/usage.rst Outdated Show resolved Hide resolved

agronholm reviewed Mar 27, 2025

View reviewed changes

docs/versionhistory.rst Outdated Show resolved Hide resolved

agronholm reviewed Apr 16, 2025

View reviewed changes

source/encoder.c Outdated Show resolved Hide resolved

source/encoder.c Outdated Show resolved Hide resolved

source/encoder.c Show resolved Hide resolved

CZDanol commented Apr 20, 2025

View reviewed changes

CZDanol and others added 2 commits May 15, 2025 11:00

Support encoding indefinite containers

b3c005c

Add the indefinite_containers parameter to the encoder functions. If the parameter is set to True, containers (maps and arrays) are encoded an indefinite containers.

Update docs/versionhistory.rst

5996066

CZDanol force-pushed the indefinite-containers branch from aba28eb to 5996066 Compare May 15, 2025 09:00

Fixed indentation

1aa0f99

agronholm merged commit 4c3c256 into agronholm:master Jun 5, 2025
13 of 14 checks passed

Support encoding indefinite containers #256

Support encoding indefinite containers #256

Uh oh!

Conversation

CZDanol commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

agronholm commented Mar 26, 2025

Uh oh!

CZDanol commented Mar 26, 2025

Uh oh!

agronholm commented Mar 26, 2025

Uh oh!

CZDanol commented Mar 26, 2025

Uh oh!

CZDanol commented Mar 26, 2025

Uh oh!

agronholm commented Mar 26, 2025

Uh oh!

coveralls commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CZDanol commented Mar 26, 2025

Uh oh!

Uh oh!

Uh oh!

agronholm commented Mar 27, 2025

Uh oh!

CZDanol commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agronholm commented Mar 28, 2025

Uh oh!

CZDanol commented Mar 28, 2025

Uh oh!

agronholm commented Mar 31, 2025

Uh oh!

CZDanol commented Mar 31, 2025

Uh oh!

agronholm commented Mar 31, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CZDanol Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agronholm May 14, 2025

Choose a reason for hiding this comment

Uh oh!

CZDanol May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agronholm May 14, 2025

Choose a reason for hiding this comment

Uh oh!

CZDanol May 15, 2025

Choose a reason for hiding this comment

Uh oh!

CZDanol May 15, 2025

Choose a reason for hiding this comment

Uh oh!

CZDanol commented May 13, 2025

Uh oh!

agronholm commented May 13, 2025

Uh oh!

CZDanol commented May 15, 2025

Uh oh!

agronholm commented May 15, 2025

Uh oh!

CZDanol commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

agronholm commented Jun 5, 2025

Uh oh!

CZDanol commented Jun 5, 2025

CZDanol commented Mar 26, 2025 •

edited

Loading

coveralls commented Mar 26, 2025 •

edited

Loading

CZDanol commented Mar 28, 2025 •

edited

Loading

CZDanol Apr 20, 2025 •

edited

Loading

CZDanol May 14, 2025 •

edited

Loading

CZDanol commented May 15, 2025 •

edited

Loading