-
Notifications
You must be signed in to change notification settings - Fork 87
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
fix: timedelta parsing for int and floats #368
base: main
Are you sure you want to change the base?
Conversation
070d1a1
to
e75ea00
Compare
e75ea00
to
1dce09a
Compare
milliseconds=int(match.group(6) or 0), | ||
microseconds=int(match.group(7) or 0), | ||
seconds=seconds, | ||
milliseconds=milliseconds, | ||
) | ||
return super()._deserialize(value, *args, **kwargs) |
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 like a lot of logic was added just to bypass the parent class' validation (this line won't be hit anymore).
- a float as default value was previously slipping through the cracks (silent downcast to int before #366, a bug not a feature imo)
- a float-in-string as default value didn't slip through the cracks (expected behaviour to reject)
is this PR something that should rather fold into the bigger topic of #270 and/or #297 instead?
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.
opened a minimal version to avoid breaking change: #369
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.
(this line won't be hit anymore).
It will if you provide something that is of other type than str, float, int, bool, timedelta.
So this is more a last option catch-all.
We could definitely do the more minimal change to get to the previously working (but unconsistent/unlogical behavior)... but I'm not sure what's wrong with this PR which supports the previously working pattern and actually fixes some of the inconsistent behaviors?
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.
and actually, I think that #297 is not relevant anymore as it was already fixed (prior to this PR)
merged solution in #369 |
Sorry but I don't understand why this is closed @sloria . The other PR solves the breaking change that was introduced... but it persist the inconsistent behavior from before. So why close this proposal of an alternative that makes the overall user experience better? |
We can re-open this for now to resume discussion on the changes that go beyond the fix. My reasons for closing--take them or leave them--were:
|
Version 11.2.0 introduced a regression when it comes to providing timedelta as floats or integers.
This MR aims at making those 4 cases valid