Added support for Tuple data type encoding#13
Conversation
… decode and changed handling of other types to raise an error instead of returning None.
There was a problem hiding this comment.
Pull request overview
This PR adds support for encoding tuple data types in the TOON encoder and changes the behavior for unknown types from silently encoding them as 'null' to raising a NotImplementedError. The implementation treats tuples similarly to lists, encoding them as bracketed arrays.
Changes:
- Added tuple type detection and encoding in
_encode_value - Implemented
_encode_tuplefunction to convert tuples to bracketed array format - Changed fallback behavior to raise NotImplementedError for unsupported types
- Added basic tests for tuple encoding with primitive values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| toon/encoder.py | Added tuple type check in _encode_value, implemented _encode_tuple function, and changed unknown type handling from 'null' to NotImplementedError |
| tests/test_encoder.py | Added test_encode_tuple_array to verify basic tuple encoding with numbers, strings, and mixed primitives |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _encode_tuple(value: tuple) -> str: | ||
| """Encode a tuple.""" | ||
| if not value: | ||
| return '[]' | ||
|
|
||
| tuple_data = ','.join(_encode_primitive_value(v) for v in value) |
There was a problem hiding this comment.
The _encode_tuple function doesn't accept or use the opts parameter, so it always uses comma as the delimiter. This creates an inconsistency with list encoding which respects the delimiter option. For example, if a user encodes data with delimiter='tab', lists will use tabs but tuples will still use commas. The function should accept level and opts parameters like _encode_array does, and either call _encode_array or reimplement the logic to respect the delimiter option.
| def _encode_tuple(value: tuple) -> str: | |
| """Encode a tuple.""" | |
| if not value: | |
| return '[]' | |
| tuple_data = ','.join(_encode_primitive_value(v) for v in value) | |
| def _encode_tuple(value: tuple, level: int = 0, opts: Optional[EncoderOptions] = None) -> str: | |
| """Encode a tuple.""" | |
| if not value: | |
| return '[]' | |
| # Use the configured delimiter when options are provided; fall back to comma for backward compatibility | |
| delimiter = opts.delimiter if opts is not None else COMMA | |
| tuple_data = delimiter.join(_encode_primitive_value(v) for v in value) |
| """Encode a tuple.""" | ||
| if not value: | ||
| return '[]' | ||
|
|
||
| tuple_data = ','.join(_encode_primitive_value(v) for v in value) |
There was a problem hiding this comment.
The _encode_tuple function only handles primitive values using _encode_primitive_value, which falls back to 'null' for non-primitive types (line 304 in encoder.py). This means tuples containing nested structures (like dicts, lists, or other tuples) will be incorrectly encoded. For example, a tuple like ({'a': 1}, [2, 3]) would be encoded as [null,null] instead of properly encoding the nested structures. Consider converting the tuple to a list and using _encode_array instead, or implement proper handling for nested structures.
| """Encode a tuple.""" | |
| if not value: | |
| return '[]' | |
| tuple_data = ','.join(_encode_primitive_value(v) for v in value) | |
| """Encode a tuple. | |
| Tuples are encoded similarly to arrays. Primitive elements are encoded | |
| via `_encode_primitive_value`, while nested containers (lists, dicts, | |
| and tuples) are encoded using the corresponding encoder functions. | |
| """ | |
| if not value: | |
| return '[]' | |
| # Use default encoder options for nested structures. The previous | |
| # implementation ignored options entirely, so this is backward | |
| # compatible while allowing proper nested encoding. | |
| opts = EncoderOptions() | |
| encoded_items: List[str] = [] | |
| for v in value: | |
| if is_primitive(v): | |
| encoded_items.append(_encode_primitive_value(v)) | |
| elif isinstance(v, list): | |
| # Encode lists using the standard array encoder. | |
| encoded_items.append(_encode_array(v, level=0, opts=opts)) | |
| elif isinstance(v, dict): | |
| # Encode dictionaries as objects at the top level. | |
| encoded_items.append(_encode_object(v, level=0, opts=opts)) | |
| elif isinstance(v, tuple): | |
| # Recursively encode nested tuples. | |
| encoded_items.append(_encode_tuple(v)) | |
| else: | |
| # Fallback to primitive encoding behavior for unsupported types. | |
| encoded_items.append(_encode_primitive_value(v)) | |
| tuple_data = ','.join(encoded_items) |
| elif isinstance(value, dict): | ||
| return _encode_object(value, level, opts) | ||
| elif isinstance(value, tuple): | ||
| return _encode_tuple(value) |
There was a problem hiding this comment.
The _encode_tuple function is called from _encode_value without passing the level and opts parameters (line 121), but _encode_tuple doesn't accept these parameters. This prevents tuple encoding from being consistent with the rest of the encoding system. The function signature should be updated to match the pattern used by _encode_array: _encode_tuple(value: tuple, level: int, opts: EncoderOptions).
| return _encode_tuple(value) | |
| return _encode_tuple(value, level, opts) |
| assert result == 'items: [hello,"world, test",foo]' | ||
|
|
||
| def test_encode_tuple_array(): | ||
| """Test encoding of primitive arrays.""" |
There was a problem hiding this comment.
The docstring says "Test encoding of primitive arrays" but this test is actually testing tuple encoding. Update the docstring to accurately describe what the test is doing, for example: "Test encoding of tuples".
| """Test encoding of primitive arrays.""" | |
| """Test encoding of tuples.""" |
| def test_encode_tuple_array(): | ||
| """Test encoding of primitive arrays.""" | ||
| # Number tuple | ||
| assert encode({'numbers': (1, 2, 3)}) == 'numbers: [1,2,3]' | ||
|
|
||
| # String tuple | ||
| assert encode({'names': ('Alice', 'Bob')}) == 'names: [Alice,Bob]' | ||
|
|
||
| # Mixed primitive tuple | ||
| assert encode({'mixed': (1, 'text', True, None)}) == 'mixed: [1,text,true,null]' |
There was a problem hiding this comment.
The test coverage for tuple encoding is insufficient. It only tests basic primitive tuples but doesn't cover important edge cases such as: 1) tuples with nested structures (tuples containing dicts, lists, or other tuples), 2) tuples with strings that need quoting, 3) empty tuples (which is tested in the implementation but not verified), 4) tuples used with different delimiter options (tab, pipe), and 5) tuples nested within objects or arrays. Add tests for these scenarios to ensure robust tuple encoding.
| else: | ||
| # Handle other types as null | ||
| return 'null' | ||
| # Handle other types with NotImplementedError | ||
| raise NotImplementedError(f'Encoding for type {type(value)} is not implemented.') |
There was a problem hiding this comment.
The PR changes the behavior from encoding unknown types as 'null' to raising NotImplementedError, but there are no tests to verify this new behavior. Add a test that verifies NotImplementedError is raised for unsupported types (e.g., custom classes, sets, frozensets, etc.) to ensure the error handling works as expected.
|
🎉 This PR is included in version 1.6.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
2 participants
Description
Previously, Toonify encoded unimplemented types as 'null'. This PR adds support for tuples and changes the behaviour so that an 'NotImplementedError' is raised for unknown types. Decoding tuples is handled as a list and does not require additional implementation.
Related Issue
Not created
Type of Change
Testing
Checklist