Uh oh!
There was an error while loading. Please reload this page.
feat: Add JSON indentation option to decode() method - #37
Conversation
## Description Implements Issue toon-format#10 by adding optional JSON indentation support to the decode() function. Users can now request JSON-formatted output with configurable indentation by passing a json_indent parameter to DecodeOptions. ## Changes Made - Added json_indent parameter to DecodeOptions class - Updated decode() to return JSON string when json_indent is specified - Enhanced docstrings with usage examples - Added 11 comprehensive tests covering all use cases ## Type of Change - [x] New feature (non-breaking change that adds functionality) ## SPEC Compliance - [x] Non-breaking change (default behavior unchanged) - [x] Backward compatible (json_indent=None by default) - [x] Python-specific feature (output formatting enhancement) ## Testing - [x] All existing tests pass - [x] Added new tests (11 total, 100% pass rate) - [x] Comprehensive coverage (basic, nested, arrays, unicode, edge cases) - [x] No breaking changes ## Code Quality - [x] All type hints present (mypy passes) - [x] Ruff linting passes (0 errors) - [x] Code formatted (ruff format) - [x] Python 3.8+ compatible ## Example Usage ```python from toon_format import decode, DecodeOptions # Default behavior - returns Python object result = decode("name: Alice\nage: 30") # {'name': 'Alice', 'age': 30} # With JSON indentation - returns formatted JSON string result = decode("name: Alice\nage: 30", DecodeOptions(json_indent=2)) # {\n "name": "Alice",\n "age": 30\n} ```
There was a problem hiding this comment.
Pull Request Overview
This PR adds optional JSON indentation support to the decode() function, allowing users to request JSON-formatted output with configurable indentation instead of returning Python objects. This is a Python-specific enhancement that maintains backward compatibility.
Key changes:
- Added
json_indentparameter toDecodeOptionsclass for output formatting control - Modified
decode()to conditionally return JSON strings whenjson_indentis specified - Added comprehensive test coverage with 11 new tests covering various edge cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/toon_format/types.py | Added json_indent parameter to DecodeOptions with detailed documentation |
| src/toon_format/decoder.py | Implemented JSON formatting logic and updated return type from JsonValue to Any |
| tests/test_api.py | Added 11 comprehensive tests for the new JSON indentation feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Per Copilot review, import statements should be at the module level for consistency and readability. Moved all 'import json' statements from inside test functions to the top of test_api.py. - Added 'import json' to module imports (line 13) - Removed 3 inline imports from test functions - All tests still passing (11/11) - Code quality checks pass (ruff, mypy)
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Removed 11 redundant hardcoded test cases and added 19 parametrized tests that validate json_indent against [TOON spec fixtures](https://github.com/toon-format/spec/tree/main/tests/fixtures/decode). The feature is now validated across 160+ spec test cases covering primitives, arrays, objects, nested structures, Unicode, emoji, escape sequences, different indent sizes and edge cases. Single source of truth: official TOON specification fixtures. Addresses maintainer concern about test coverage while maintaining code quality and reducing test duplication.
Refactored to import and reuse get_all_decode_fixtures() from test_spec_fixtures.py instead of duplicating fixture loading logic. This reduces code duplication and ensures both test modules use the same official TOON spec fixtures.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Removed empty TestDecodeJSONIndentation class (was adding unnecessary code) - Fixed flawed assertion logic in test_json_indent_with_different_indent_sizes - Changed return type from Any to Union[JsonValue, str] for better type safety (makes it clear the function returns either a Python value or a JSON string) - Added Union to typing imports in decoder.py All 19 spec-based json_indent tests passing ✓
Kept remote version of TestDecodeJSONIndentation with @pytest.mark.skip decorator as suggested by Copilot review. This is a better approach than removing the class entirely.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix bug where ToonDecodeError from decode_object was incorrectly caught when checking if input is a key-value pair, causing unterminated strings to be treated as root primitives instead of raising an error - Refactor to use try/except/else pattern consistent with codebase style - Fix linting issues in test_api.py: - Fix import sorting - Fix line length violations - Remove trailing whitespace from blank lines - Apply code formatting
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pass | ||
| def _get_sample_decode_fixtures() -> List[tuple]: |
There was a problem hiding this comment.
The return type hint for _get_sample_decode_fixtures() is List[tuple] which is imprecise. Based on how the return value is used (lines 337-338 and 365-366), it should be List[Tuple[str, Dict[str, Any]]] to match the structure of the tuples being appended at line 325.
Consider updating the import and return type:
fromtypingimportAny, Dict, List, Tuple# Add Tuple to importsdef_get_sample_decode_fixtures() ->List[Tuple[str, Dict[str, Any]]]:| # If json_indent is specified, return JSON-formatted string | ||
| if options.json_indent is not None: | ||
| if options.json_indent < 0: | ||
| raise ToonDecodeError(f"json_indent must be non-negative, got {options.json_indent}") |
There was a problem hiding this comment.
The error raised for negative json_indent values uses ToonDecodeError, but the docstring documents ValueError. Since json_indent is a formatting option unrelated to TOON format parsing, raising ValueError would be more semantically appropriate and consistent with the documented behavior.
Consider changing to:
raiseValueError(f"json_indent must be non-negative, got {options.json_indent}")| raiseToonDecodeError(f"json_indent must be non-negative, got {options.json_indent}") | |
| raiseValueError(f"json_indent must be non-negative, got {options.json_indent}") |
| indent: Number of spaces per indentation level (default: 2) | ||
| Used for parsing TOON format. | ||
| strict: Enable strict validation (default: True) | ||
| Enforces spec conformance checks. |
There was a problem hiding this comment.
[nitpick] The comment on lines 55-57 uses inconsistent terminology compared to the rest of the codebase. Line 55 says "Used for parsing TOON format" while line 57 says "Enforces spec conformance checks." The wording for indent could be more precise to match the style of the strict parameter.
Consider revising for consistency:
indent: Numberofspacesperindentationlevel (default: 2).
UsedforparsingTOONformatinput.
strict: Enablestrictvalidation (default: True).
EnforcesTOONspecificationconformancechecks.| indent: Numberofspacesperindentationlevel (default: 2) | |
| UsedforparsingTOONformat. | |
| strict: Enablestrictvalidation (default: True) | |
| Enforcesspecconformancechecks. | |
| indent: Numberofspacesperindentationlevel (default: 2). | |
| strict: Enablestrictvalidation (default: True). | |
| EnforcesTOONspecificationconformancechecks. |
| @pytest.mark.skip( | ||
| reason="Placeholder for targeted decode() JSON indentation tests. See TODO above." | ||
| ) | ||
| class TestDecodeJSONIndentation: | ||
| """Test decode() JSON indentation feature (Issue #10). | ||
| Comprehensive tests for the json_indent feature are in | ||
| TestDecodeJSONIndentationWithSpecFixtures, which validates against official | ||
| TOON specification fixtures. | ||
| """ | ||
| pass | ||
There was a problem hiding this comment.
[nitpick] The class TestDecodeJSONIndentation is marked with @pytest.mark.skip but contains only a pass statement. This creates dead code that provides no value.
Since the TODO comment indicates that "comprehensive tests are in TestDecodeJSONIndentationWithSpecFixtures," and that class already exists with actual tests, this entire empty skipped class should be removed to keep the codebase clean. If targeted unit tests are needed in the future, they can be added at that time.
| @pytest.mark.skip( | |
| reason="Placeholder for targeted decode() JSON indentation tests. See TODO above." | |
| ) | |
| classTestDecodeJSONIndentation: | |
| """Testdecode() JSONindentationfeature (Issue#10). | |
| Comprehensivetestsforthejson_indentfeaturearein | |
| TestDecodeJSONIndentationWithSpecFixtures, whichvalidatesagainstofficial | |
| TOONspecificationfixtures. | |
| """ | |
| pass |
| for test_id, test_data, fixture_name in all_fixtures: | ||
| if f"{fixture_name}.json" in selected_files and len(test_cases) < 9: | ||
| test_cases.append((test_id, test_data)) |
There was a problem hiding this comment.
The unpacking in this loop is incorrect. get_all_decode_fixtures() returns tuples of (test_id, test_data, fixture_name) (3 elements), but the code unpacks only 2 elements into (test_id, test_data). This will cause a ValueError: too many values to unpack at runtime.
Fix the unpacking to include all three elements:
fortest_id, test_data, fixture_nameinall_fixtures:
iff"{fixture_name}.json"inselected_filesandlen(test_cases) <9:
test_cases.append((test_id, test_data))- Add Tuple to typing imports for better type precision - Update _get_sample_decode_fixtures() return type from List[tuple] to List[Tuple[str, Dict[str, Any]]] - Improves code clarity and type safety for static analysis tools
Description
Implements Issue #10 by adding optional JSON indentation support to the decode() function. Users can now request JSON-formatted output with configurable indentation.
Changes Made
1. src/toon_format/types.py
json_indent: Union[int, None] = Noneparameter to DecodeOptions class2. src/toon_format/decoder.py
import jsonfor JSON serialization3. tests/test_api.py
Type of Change
SPEC Compliance
Testing
✅ All 11 new tests pass
✅ All existing tests pass (47 passing in test_api.py)
✅ Code quality: ruff check passes, mypy passes
✅ No breaking changes
Code Quality
Example Usage
Checklist
Closes#10