Clarify TimestampBuilder.build_at_offset #282
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this commit, `TimestampBuilder.build_at_offset` claimed to take minutes but actually took "seconds west of UTC". Now, we take "minutes east of UTC" which is more inline with both intuition (at least, my own!) and how Ion represents the value in binary. Note that the `Timestamp` type uses the chrono types internally (such as an offset in seconds). This creates some annoying multiply/divide by 60. However, given that Ion doesn't actually support representing offset in seconds, it feels right to me to use minutes in our API. Maybe, in future work, we should more clearly restrict offsets to minutes and not use the chrono type. Fixes amazon-ion#190.
Codecov Report
@@ Coverage Diff @@
## main #282 +/- ##
==========================================
- Coverage 91.78% 91.78% -0.01%
==========================================
Files 56 56
Lines 8287 8285 -2
==========================================
- Hits 7606 7604 -2
Misses 681 681
Continue to review full report at Codecov.
|
marcbowes
commented
Jun 10, 2021
| // Our builder stores the offset in minutes difference from UTC. | ||
| // We're about to convert to the chrono type which uses seconds. | ||
| // Both values consider positive to mean Eastern Hemisphere. | ||
| let offset = FixedOffset::east_opt(offset * 60).ok_or_else(|| { |
There was a problem hiding this comment.
Note west -> east.
marcbowes
commented
Jun 10, 2021
Comment on lines
+686
to
+687
| let timestamp1 = builder.clone().build_at_offset(5 * 60)?; | ||
| let timestamp2 = builder.clone().build_at_offset(5 * 60)?; |
There was a problem hiding this comment.
All these tests lost a minutes -> seconds conversion.
marcbowes
commented
Jun 10, 2021
| @@ -1012,7 +1014,7 @@ mod ionc_tests { | |||
| ), | |||
| Timestamp::with_ymd(2020, 1, 1) | |||
| .with_hour_and_minute(0, 1) | |||
| .build_at_offset(8 * 60 * 60) | |||
| .build_at_offset(-8 * 60) | |||
There was a problem hiding this comment.
This (and the next) test needed a sign update.
almann
approved these changes
Jun 11, 2021
zslayton
approved these changes
Jun 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #190
Description of changes:
Before this commit,
TimestampBuilder.build_at_offsetclaimed to takeminutes but actually took "seconds west of UTC". Now, we take "minutes
east of UTC" which is more inline with both intuition (at least, my
own!) and how Ion represents the value in binary.
Note that the
Timestamptype uses the chrono types internally (such asan offset in seconds). This creates some annoying multiply/divide by 60.
However, given that Ion doesn't actually support representing offset in
seconds, it feels right to me to use minutes in our API. Maybe, in
future work, we should more clearly restrict offsets to minutes and not
use the chrono type.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.