Skip to content

Implement datetime timezone constraints #343

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

Conversation

antonagestam
Copy link
Contributor

@antonagestam antonagestam commented Dec 3, 2022

Alright, so I got something together that is seemingly working. Please let me know if you think something should be done differently, or structured in another way.

This implements support for constraining datetime objects based on them having or not having timezone info. The aware kind constrains to objects that have timezone info, and symmetrically, the naive kind constrains to objects that do not have timezone info.

Closes #266.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 3, 2022

CodSpeed Performance Report

Merging #343 feature/implement-datetime-timezone-constraints (9dccdea) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 65 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

@antonagestam antonagestam marked this pull request as draft December 3, 2022 12:12
@antonagestam
Copy link
Contributor Author

Converted to draft, looking at the test failures.

@antonagestam antonagestam force-pushed the feature/implement-datetime-timezone-constraints branch from 05781c1 to 2429637 Compare December 3, 2022 12:21
@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2022

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.13%. Comparing base (bb28794) to head (9dccdea).
Report is 784 commits behind head on main.

Files Patch % Lines
src/validators/datetime.rs 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   96.34%   97.13%   +0.78%     
==========================================
  Files          58       58              
  Lines        6953     6978      +25     
  Branches       47       47              
==========================================
+ Hits         6699     6778      +79     
+ Misses        252      200      -52     
+ Partials        2        0       -2     
Files Coverage Δ
pydantic_core/core_schema.py 100.00% <100.00%> (+11.38%) ⬆️
src/errors/types.rs 77.63% <ø> (ø)
src/validators/datetime.rs 93.33% <89.47%> (-0.79%) ⬇️

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb28794...9dccdea. Read the comment docs.

@antonagestam
Copy link
Contributor Author

@samuelcolvin Any idea what's going on with the wasm build? Do we need to specify somewhere to also build the zoneinfo standard library or something?

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

looks to me like the wasm failures are a problem with hypothesis trying to import tzinfo, not relelated to the tests here.

You could be able to check this by adding conventional tests for this behaviour, then mark the hypothesis tests as skipped on wasm

@samuelcolvin
Copy link
Member

overall, this looks great - doesn't look like that much to change.

@antonagestam antonagestam force-pushed the feature/implement-datetime-timezone-constraints branch from 2429637 to 8c76666 Compare December 4, 2022 16:07
This implements support for constraining datetime objects based on them
having or not having timezone info. The aware kind constrains to objects
that have timezone info, and symmetrically, the naive kind constrains to
objects that do not have timezone info.
@antonagestam antonagestam force-pushed the feature/implement-datetime-timezone-constraints branch from 8c76666 to 9dccdea Compare December 4, 2022 16:11
@antonagestam antonagestam marked this pull request as ready for review December 4, 2022 16:14
@antonagestam
Copy link
Contributor Author

I simplified both tests and merged the two types as suggested.

Overall, even though this is a very small change, very enjoyable to work with Rust I must say! 👍

@samuelcolvin samuelcolvin merged commit 9973cee into pydantic:main Dec 4, 2022
@samuelcolvin
Copy link
Member

This is great, thanks so much.

Pleased you enjoyed it, I'd love you to contribute more if you have the time.

@antonagestam antonagestam deleted the feature/implement-datetime-timezone-constraints branch December 4, 2022 17:15
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.

naive and aware constraints on datetime types
3 participants