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 NaiveDateTime and AwareDateTime #1278

Merged
merged 15 commits into from Jul 17, 2019
Merged

Add NaiveDateTime and AwareDateTime #1278

merged 15 commits into from Jul 17, 2019

Conversation

lafrech
Copy link
Member

@lafrech lafrech commented Jul 2, 2019

Closes #1234.

@sloria
Copy link
Member

sloria commented Jul 3, 2019

Is NaiveDateTime necessary? What's the use case?

@lafrech
Copy link
Member Author

lafrech commented Jul 3, 2019

In Python, naive and aware datetimes are different objects with different properties and some operations only work on either naive or aware datetimes. I was caught by this already (https://github.com/lepture/flask-oauthlib/issues/294). Being able to enforce the type is therefore a must have IMHO. Otherwise, the user needs to do it manually.

Whether it is done by a specific class or a flag is an implementation choice. I think different classes is better, more explicit. Generally we have a class for a type and the flags are just parameters.

@sloria
Copy link
Member

sloria commented Jul 3, 2019

It makes sense to have validation that an input datetime includes timezone info, but I'm more curious about cases when you want to enforce naiveness. Or is it more about having symmetry?

@lafrech
Copy link
Member Author

lafrech commented Jul 3, 2019

Symmetry would be good enough a reason for me, since it is relatively easy to achieve.

This issue is an example of a lib that expects a naive datetime and will fail if fed an aware datetime. I don't use it anymore, but by the time, I had to enforce naiveness myself manually.

This issue can be extended to every use case involving date comparisons.

If the API stores datetimes, it most likely wants to enforce the type, if only to allow the user to sort objects by datetime. Perhaps most of the time, the DB driver will uniformize the format, by making a silent choice to force aware or naive, but I think this should be the role of the deserialization layer.

Maybe I'm missing something because once I fell into this, I figured it was something marshmallow absolutely needed to address, but since then, I heard no one complain, so maybe it is not such an issue in practice.

(In any case, I'd still consider this PR relevant, as it comes with a nice cleanup, hopefully no regression, and a code easier to maintain and with better test coverage.)

@sloria
Copy link
Member

sloria commented Jul 3, 2019

Sure, I'm fine with that reasoning and with going forward with this. Will take a closer look hopefully over the next few days.

@deckar01
Copy link
Member

deckar01 commented Jul 3, 2019

I heard no one complain

I suspect loading datetimes is just not a very common behavior of most apps. Not until I made a webcam timelapse scheduler recently did I ever have to accept a datetime from the user.

Copy link
Member

@sloria sloria left a comment

Choose a reason for hiding this comment

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

I'm basically good with this. My comments are nits. Once this is polished up, should be good to merge. 🎉

src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/utils.py Outdated Show resolved Hide resolved
src/marshmallow/utils.py Outdated Show resolved Hide resolved
tests/test_deserialization.py Outdated Show resolved Hide resolved
@lafrech lafrech force-pushed the 1234_rework_datetime branch 3 times, most recently from 3778b15 to c1bc43c Compare July 12, 2019 15:04
@lafrech lafrech marked this pull request as ready for review July 12, 2019 15:15
@lafrech
Copy link
Member Author

lafrech commented Jul 12, 2019

Rebased and documented.

Feedback welcome.

I'm open to changes, especially about the new parameter names. And the docs.

Copy link
Member

@sloria sloria left a comment

Choose a reason for hiding this comment

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

Looking awesome. Nice job with the tests. Just nits left.

src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
tests/test_schema.py Outdated Show resolved Hide resolved
tests/test_schema.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
src/marshmallow/fields.py Outdated Show resolved Hide resolved
@lafrech
Copy link
Member Author

lafrech commented Jul 16, 2019

Rebased. Remarks addressed.

Copy link
Member

@sloria sloria left a comment

Choose a reason for hiding this comment

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

Very nice. Can you update the changelog then merge?

🚢 🇮🇹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateTime timezone management rework
3 participants