-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
[8570]fix: raise exception for years less than 1600 #55
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
=======================================
Coverage 99.12% 99.13%
=======================================
Files 6 6
Lines 918 922 +4
=======================================
+ Hits 910 914 +4
Misses 8 8
Continue to review full report in Codecov by Sentry.
|
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'm afraid I don't agree with this change, nor the suggestion that raising a more informative error is the correct fix.
I think most likely the source of the panic is on the following line, where year
is a u16
so subtracting 1600 from it is incorrect for years under 1600
Line 231 in ef53708
let days = (self.year - 1600) as i64 * 365 |
Probably it would be more correct as (self.year as i64 - 1600) * 365
, as i64
can go below 0.
There's a slight follow up question of what the limits of that timestamp()
function would be, probably operations in that function which we think can overflow should use .checked_mul()
/ .checked_sub()
if year < 1600 { | ||
return Err(ParseError::YearTooSmall); | ||
} | ||
|
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.
Adding this here will break pydantic's ability to parse all dates before 1600, whereas currently the issue only exists for FutureDatetime
(and I guess PastDatetime
).
Superseded by #77 |
fix pydantic/pydantic#8570