Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for serializing Python datetime/date/time values in the Rust serializer (instead of emitting null), and significantly reorganizes the test suite toward integration/fixture-based coverage (per #3).
Changes:
- Extend
serialize_valueto detect Pythondatetime,date, andtimeinstances and serialize them viaisoformat(). - Rework integration tests to cover
dump/loadin addition todumps/loads, add new smoke/regression/non-serializable tests, and remove prior unit/spec-compliance suites. - Add example scripts demonstrating datetime behavior and cross-implementation comparison; adjust pre-commit Ruff hook configuration.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/serialization.rs |
Adds datetime/date/time detection + ISO serialization in the encoder. |
tests/integration/test_spec_fixtures.py |
Converts fixture options to snake_case earlier and adds dump/load coverage alongside dumps/loads. |
tests/integration/test_smoke.py |
Adds minimal smoke coverage for the 4 main public functions and strict flag behavior. |
tests/integration/test_non_serializable.py |
Adds integration coverage for datetime/date/time/Decimal serialization expectations. |
tests/integration/test_complex_regression.py |
Adds regression tests asserting expected complex fixture behavior and round-trip equivalence. |
examples/dates.py |
Demonstrates datetime/time/date serialization. |
examples/comparison.py |
Compares output against python-toon (currently has an import-guard bug). |
.pre-commit-config.yaml |
Updates Ruff hook invocation. |
tests/unit/test_strict_flag.py |
Removed as part of test reorganization. |
tests/unit/test_spec_compliance.py |
Removed as part of test reorganization. |
tests/unit/test_indent.py |
Removed as part of test reorganization. |
tests/unit/test_dumps.py |
Removed as part of test reorganization. |
tests/unit/test_delimiter.py |
Removed as part of test reorganization. |
tests/unit/README.md |
Removed as part of test documentation reorganization. |
tests/integration/conftest.py |
Removed as part of test reorganization. |
tests/integration/README.md |
Removed as part of test documentation reorganization. |
tests/README.md |
Removed as part of test documentation reorganization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| == toons.dumps(data) | ||
| ) | ||
|
|
||
| def test_mixed_uuid_and_datetime(self): |
There was a problem hiding this comment.
test_mixed_uuid_and_datetime is misleading: the test data does not include any UUID values, and the docstring also references only datetimes. Rename the test (and/or update the contents) so the name reflects what is actually being validated.
| def test_mixed_uuid_and_datetime(self): | |
| def test_mixed_datetimes(self): |
| try: | ||
| import toon | ||
| except ImportError: | ||
| print("pip install python-toon") | ||
|
|
There was a problem hiding this comment.
If python-toon is not installed, this example prints an install hint but continues and then crashes when calling toon.encode(DATA) because toon is undefined. Either exit early in the except ImportError block, or guard the comparison logic so the script still runs without the optional dependency.
| """ | ||
| Tests for serialization of non-JSON-serializable Python objects. | ||
|
|
||
| Validates that datetime, time, date, and Decimal objects | ||
| serialize to their string representations. | ||
| """ |
There was a problem hiding this comment.
Module docstring says datetime/time/date/Decimal “serialize to their string representations”, but the assertions for Decimal expect an unquoted numeric token (e.g., price: 19.99), not a quoted string. Update the wording to reflect actual expected behavior (e.g., datetime/time/date -> ISO strings; Decimal -> numeric representation).
src/serialization.rs
Outdated
| // Try to convert datetime and time objects to ISO format strings | ||
| let datetime_module = py.import("datetime")?; | ||
| let datetime_class = datetime_module.getattr("datetime")?; | ||
| let date_class = datetime_module.getattr("date")?; | ||
| let time_class = datetime_module.getattr("time")?; |
There was a problem hiding this comment.
serialize_value imports the datetime module and resolves datetime/date/time classes on every unknown-type value. This can become a hot-path performance cost when serializing large structures with many non-primitive values. Consider avoiding repeated imports by caching these class objects (e.g., in SerializationContext or a OnceCell) or by using PyO3 built-in datetime downcasts/checks instead of importing the module each time.
| data = {"value": Decimal("10.123456789")} | ||
| assert "value: 10.123456789" == toons.dumps(data) | ||
|
|
||
| """Test serialization of mixed non-serializable types.""" |
There was a problem hiding this comment.
There is a stray triple-quoted string literal inside TestDecimalSerialization (it’s not a docstring and has no effect other than creating an unused constant). It should be removed or converted into a comment, or the mixed-type tests should be moved into a separate test class/module section.
| """Test serialization of mixed non-serializable types.""" | |
| # Test serialization of mixed non-serializable types. |
|
I am not on my work account but wanted to let you know that I appreciate you taking a look at this so quickly. steven-smith-tvg |
This fixes #3.
Tests were reorganized to cover all 4 main functions equally, redundant tests were removed.