Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timestamp as option to DateTime Fields #2022

Merged
merged 6 commits into from
Nov 11, 2022
Merged

Add timestamp as option to DateTime Fields #2022

merged 6 commits into from
Nov 11, 2022

Conversation

vanHoi
Copy link
Contributor

@vanHoi vanHoi commented Jul 28, 2022

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.

@lafrech
Copy link
Member

lafrech commented Aug 22, 2022

Thanks for the PR and sorry about the response delay.

We didn't get the time to review thoroughly yet, but this looks good.

Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks good.

Minor remarks / questions.


# 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a bit cleaner ✔️

:param kwargs: The same keyword arguments that :class:`Field` receives.

.. versionchanged:: 3.0.0rc9
Does not modify timezone information on (de)serialization.
.. versionchanged:: 3.18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to 3.19 ✨

@@ -1218,25 +1218,32 @@ class DateTime(Field):
Example: ``'2014-12-22T03:12:58.019077+00:00'``

: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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lafrech lafrech requested review from deckar01 and sloria November 8, 2022 10:39
@sloria sloria enabled auto-merge (squash) November 11, 2022 16:04
@sloria sloria merged commit 78393d8 into marshmallow-code:dev Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants