Add date to preconfigured converters #420
Conversation
src/cattrs/preconf/msgpack.py
Outdated
| @@ -30,6 +30,8 @@ def configure_converter(converter: BaseConverter): | |||
| converter.register_structure_hook( | |||
| datetime, lambda v, _: datetime.fromtimestamp(v, timezone.utc) | |||
| ) | |||
| converter.register_unstructure_hook(date, lambda v: v.isoformat()) | |||
There was a problem hiding this comment.
I don't know much about msgpack, but it seems like the msgpack converter is doing things a bit differently with datetime - it is using timestamp rather than isoformat. Is there a reason the msgpack converter does this differently? How should we be handling dates in the msgpack converter?
There was a problem hiding this comment.
We can, but let's make it use timestamps too, for consistency.
The idea is for the JSON converters to use the ISO format since JSON is meant to be somewhat human readable, and JavaScript can parse them out of the box. For binary formats like msgpack I think timestamps are a better fit since they're much more efficient.
There was a problem hiding this comment.
Interesting, thanks. Updated now. Since date has no .timestamp method, I've used the midnight-aligned datetime.
|
Not sure what I have to do to get CI to run, but |
|
Thanks for picking this up. I've clicked the button to run the tests, it needs my approval I guess. |
tests/test_preconf.py
Outdated
| @@ -148,6 +150,7 @@ def everythings( | |||
| Everything.AnIntEnum.A, | |||
| Everything.AStringEnum.A, | |||
| draw(dts), | |||
| draw(dates()), | |||
There was a problem hiding this comment.
Please set the max value here similar to what the datetime strategy is doing above. If we don't do this, we might break some 32-bit Linux downstreams (don't ask...).
Peter554
force-pushed
the
add-date-to-preconfigured-converters
branch
2 times, most recently
from
ae49e79 to
872448f
Compare
Peter554
force-pushed
the
add-date-to-preconfigured-converters
branch
from
872448f to
333a911
Compare
|
Great job, thanks! |
Adding support for
dateto the preconfigured converters. I see there was already some effort made in this direction here #365, however that PR is missing tests and seems to be a bit inactive.