-
Notifications
You must be signed in to change notification settings - Fork 236
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
Update speedate and truncate microseconds by default #762
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #762 +/- ##
==========================================
+ Coverage 93.65% 93.68% +0.03%
==========================================
Files 99 99
Lines 14148 14243 +95
Branches 25 25
==========================================
+ Hits 13250 13344 +94
- Misses 892 893 +1
Partials 6 6
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #762 will improve performances by 10.08%Comparing Summary
Benchmarks breakdown
|
please review |
pub fn bytes_as_time<'a>( | ||
input: &'a impl Input<'a>, | ||
bytes: &[u8], | ||
microseconds_overflow_behavior: MicrosecondsPrecisionOverflowBehavior, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to go this way rather than config: TimeConfig
argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it and chose this because (1) it makes it clearer just from the parameter what the purpose is, (2) there is currently only 1 config option and (3) this is all private / internal so we can refactor to TimeConfig
without any breaking changes later (unlike in speedate
itself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear — we are viewing all rust code in pydantic-core as private, right? If that's the case, I'm fine with this either way. If this isn't published as a crate on cargo or whatever, that seems fine. If we did want to make it public, I'd be more inclined to use the config: TimeConfig
approach, but I guess I prefer Adrian's approach if we can make breaking changes to this API with no consequences because I would agree it does do a better job of conveying precisely what configuration is meant to be controlled here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Unless there are other options we could/should expose from TimeConfig
, in which case, maybe that makes more sense. I haven't looked into it and don't intend to unless one of you suggests I should.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the python package is public (although I don't think it should be and I don't think anyone is depending on it) but the Rust code is 100% private
match Time::parse_bytes_with_config( | ||
bytes, | ||
TimeConfig { | ||
microseconds_precision_overflow_behavior: microseconds_overflow_behavior, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well speak of the devil, if this is the only field currently, I think it's fine to just use the signature above that accepts a MicrosecondsPrecisionOverflowBehavior
, (unless, as noted, this API really is public in some way).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I didn't think hard about the changes to pub
etc. which I am assuming are fine if it compiles.
This approval is also predicated on the assumption that the "public API" within the Rust code is really only for internal usage in pydantic-core, and not something we make any promises about to any theoretical downstream consumers.
Selected Reviewer: @dmontagu