-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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: equivalent negative durations add to the same time #43795
Conversation
38e7c1d
to
4326b10
Compare
If buildkite is able to reproduce the test failure on:
I'll add cpb@f12a0fa which provides a fix. |
activesupport (2.5) ❌ |
Instead of calling |
10196cc
to
bfd0d08
Compare
For reference @rafaelfranca #43809 was put up to place-hold an alternative option for fix. Given the preference for that approach @cpb has kindly brought that work over into this PR. I will close #43809 now. |
@rafaelfranca if it helps, I created a PR for 7-0-stable as well |
Let me know if you want one for |
All PRs should target main, then can be backported to old versions as appropriate. Could you update this one to target main? |
Yes! Thanks for that clarification 😊 |
Co-authored-by: Caleb <me@cpb.ca> Co-authored-by: Braden Staudacher <braden.staudacher@chime.com>
bfd0d08
to
5c8d638
Compare
@ghiculescu updated to main, renamed other PR's for backporting. Or, should I close them? |
I think just close them. Core team will decide on backports when they review this one. |
I think this needs a CHANGELOG entry too. |
@ghiculescu Done! 🙌 |
* bug: illustrate negative durations don't add to the same time * fix: equivalent negative durations add to the same time Co-authored-by: Caleb <me@cpb.ca> Co-authored-by: Braden Staudacher <braden.staudacher@chime.com> * Updates CHANGELOG with fix to `ActiveSupport::Duration.build` Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
* bug: illustrate negative durations don't add to the same time * fix: equivalent negative durations add to the same time Co-authored-by: Caleb <me@cpb.ca> Co-authored-by: Braden Staudacher <braden.staudacher@chime.com> * Updates CHANGELOG with fix to `ActiveSupport::Duration.build` Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
Summary
Modulo does not work as needed for negative numbers. (thanks @bradenchime for digging into that).
This PR avoids sending negative values into the modulo based parts calculating algorithm of
ActiveSupport::Duration.build
Context
I want to help the next person who attempts to use
ActiveSupport::Duration.build
and has negative numbers slip into there in their use case.This PR makes
ActiveSupport::Duration.build(-1)
not only== -ActiveSupport::Duration.build(1)
but also results in the sameTime
when added.I've noticed, in the closed Issues related to
ActiveSupport::Duration
that there is counter intuitive behaviour associated with duration math. However, because-1.second
or evenActiveSupport::Duration::Scalar.new(-1)
modifiesTime
as I expectedActiveSupport::Duration.build(-1)
to, I thought: perhaps this is unexpected behaviour.Alternatives
If it is decided to not change the behaviour of
ActiveSupport::Duration.build(-1)
then I humbly propose changing my lastassert_equal
torefute_equal
and adding (at least) documentation toActiveSupport::Duration.build
that it is not expected to work with negative values, and to useNumeric#second
orActiveSupport::Duration::Scalar
(for example) instead.Observed Error Rate
This is a single sample of the slippage or error rate in the modification of
Time
with aActiveSupport::Duration.build
with a negative value. When I complete collecting the data for the range of whole numbers in a year, I'll be able to share a chart that illustrates the range of errors.This will ultimately be a graph of the errors introduced in the algorithm by taking a modulo of a negative number.