Add timestamp as option to DateTime Fields#2022
Add timestamp as option to DateTime Fields#2022sloria merged 6 commits intomarshmallow-code:devfrom vanHoi:dev
Conversation
|
Thanks for the PR and sorry about the response delay. We didn't get the time to review thoroughly yet, but this looks good. |
lafrech
left a comment
lafrech
left a comment
There was a problem hiding this comment.
Thanks. Looks good.
Minor remarks / questions.
src/marshmallow/utils.py
Outdated
|
|
||
| # When a date is naive, use utc as zone info to prevent using system timezone. | ||
| # See Python docs for more info: https://docs.python.org/3.10/library/datetime.html#datetime.datetime.timestamp | ||
| return value.replace(tzinfo=dt.timezone.utc).timestamp() |
There was a problem hiding this comment.
I'd do it the other way around.
if not is_aware(value):
# When a date is naive, use UTC as zone info to prevent using system timezone.
value = value.replace(tzinfo=dt.timezone.utc)
return value.timestamp()There was a problem hiding this comment.
I agree this is a bit cleaner ✔️
src/marshmallow/fields.py
Outdated
|
|
||
| .. versionchanged:: 3.0.0rc9 | ||
| Does not modify timezone information on (de)serialization. | ||
| .. versionchanged:: 3.18 |
|
|
||
| :param format: Either ``"rfc"`` (for RFC822), ``"iso"`` (for ISO8601), | ||
| or a date format string. If `None`, defaults to "iso". | ||
| ``"timestamp"``, ``"timestamp_ms"`` (for a POSIX timestamp) or a date format string. |
There was a problem hiding this comment.
Wondering (out loud) about the choices and their naming. I don't use timestamps. Are those two the two common uses?
timestamp is a number of seconds. Should we call it timestamp_s? I don't think so. Why the need for timestamp_ms (and not other units)? Is it also commonly used?
There was a problem hiding this comment.
Thanks for the review! Good question. These are the two common use cases (to my knowledge). JavaScript tends to use timestamps in milliseconds, while Python uses seconds by default.
lafrech
left a comment
lafrech
left a comment
There was a problem hiding this comment.
Let's keep timestamp for seconds. We live in Python world. OK for providing timestamp_ms even if it opens the door to possibly N other unit requests (µs, ns,...), N should be small anyway.
Giving a little time for another maintainer to review before merging.
4 participants
This PR adds the possibility to load and dump to timestamps.
This was discussed before in issue #612 and then partly implemented in #1003. Since the branch was stale (still on Marshmellow 2) I decided to start over.
The difficulty with timestamps is that you can't infer a timezone from a timestamp. Since the introduction of AwareDateTime and NaiveDateTime however, this issue is mostly solved. By specifying a default timezone, the user can add the correct timezone when it is required.