adds accessor methods for `Timestamp` #482

desaikd · 2023-03-14T21:29:01Z

Issue #489

Description of changes:
This PR works on adding accessor methods for Timestamp.

List of changes:

adds year, month, day, hour, minute and second methods for given timestamp in local time.
adds utc_hour, utc_minute and utc_second methods for given timestamp in UTC time.

Test:
adds unit tests for different accessor methods defined in Timestamp.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* adds year, month, day, hour, minute and second methods for given Timetsamp in local time. * adds utc_hour, utc_minute and utc_second methods for given Timestamp in UTC time. * adds unit tests for these accessor methods.

codecov · 2023-03-14T21:31:51Z

Codecov Report

Patch coverage: 95.74% and project coverage change: +0.01 🎉

Comparison is base (0c4858c) 89.52% compared to head (1e8f911) 89.54%.

❗ Current head 1e8f911 differs from pull request most recent head f0b9ba7. Consider uploading reports for the commit f0b9ba7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
+ Coverage   89.52%   89.54%   +0.01%     
==========================================
  Files          76       76              
  Lines       13747    13845      +98     
==========================================
+ Hits        12307    12397      +90     
- Misses       1440     1448       +8

Impacted Files	Coverage Δ
src/types/timestamp.rs	`92.62% <95.74%> (+0.48%)`	⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

zslayton · 2023-03-15T14:27:55Z

src/types/timestamp.rs

+    pub fn day(&self) -> u32 {
+        self.date_time.day()
+    }


For year, month, and day, I believe the current implementation always returns the UTC date. We'll need to offer local and utc_ versions of each of those as well. (Consider 11PM on New Year's Eve in NYC, when it's already New Year's day in London.)

added to_utc for Timestamp, removed all utc_* methods.

self.date_time.day() is the UTC day. In order to return the local day, you have to localize the year/month/day fields of self.date_time to accommodate the new year's eve case I described above.

If the Timestamp doesn't have an offset because its precision is too low, you can use the current values.

zslayton · 2023-03-15T14:31:30Z

src/types/timestamp.rs

+    }
+
+    #[test]
+    fn test_timestamp_utc_hour() -> IonResult<()> {


For each unit test, please include examples that test the unit's boundary across timezone offsets. (For example, in this hour test we see 5/10/15 -- we also need to verify the behavior when the result goes backwards or forwards across the day boundaries at 0/24.)

I'm unresolving this thread because I don't see tests that cross the day boundary. If the hour is 10, what happens when the offset is -11:00? Or if it's 15 and the offset is 10:00?

src/types/timestamp.rs

jpschorr · 2023-03-15T19:00:20Z

It would be nice to have an accessor for the fractional seconds and/or milliseconds part of the timestamp as well.

src/types/timestamp.rs

popematt · 2023-03-16T19:29:08Z

src/types/timestamp.rs

+    }
+
+    /// Returns the hour(s) that has been specified in the [Timestamp].
+    pub fn hour(&self) -> u32 {


Have you thought about returning an Option from these functions so that they can reflect the precision of the timestamp?
E.g.

/// Returns the hour(s) that has been specified in the [Timestamp] if the timestamp's precision /// includes hours. If you need hour, regardless of precision, use `hour().unwrap_or_default()`, /// which will correctly return an hour of `0` when the precision does not include hours. pub fn hour(&self) -> Option<u32> { // ... }

I think it makes it match the data model a little better. It's a little more verbose when you always want some value, but it's a little more ergonomic when you only want to deal with the subfields that are included by the timestamp's precision. What do you think?

This is a good observation. I'm unsure which way to go on it; Ion Timestamps always have a hour/minutes/second in that they represent a fixed point in time and those fields are 0 when the precision is too low. However, this does represent an opportunity to remind users that the 0 might have been implicit rather than explicitly part of the value.

I suspect that as a generalized datetime value, providing field defaults is probably what users would prefer. @jpschorr, what's your take on this? Would you rather check the timestamp's precision, or hour().is_none()?

+ 1. I think it would rather simple to check precision instead of hour().is_none().

…p-accessor

src/types/timestamp.rs

popematt

FYI—there's a TODO you can get rid of on lines 162-164.

src/types/timestamp.rs

zslayton · 2023-03-17T21:11:05Z

src/types/timestamp.rs

+    pub fn day(&self) -> u32 {
+        self.date_time.day()
+    }


self.date_time.day() is the UTC day. In order to return the local day, you have to localize the year/month/day fields of self.date_time to accommodate the new year's eve case I described above.

If the Timestamp doesn't have an offset because its precision is too low, you can use the current values.

src/types/timestamp.rs

zslayton · 2023-03-17T21:20:19Z

src/types/timestamp.rs

+    }
+
+    #[test]
+    fn test_timestamp_day() -> IonResult<()> {


We need some tests for the year/month/day accessors where the Timestamp has high precision and an offset.

zslayton · 2023-03-17T21:24:04Z

src/types/timestamp.rs

+    }
+
+    #[test]
+    fn test_timestamp_utc_hour() -> IonResult<()> {


I'm unresolving this thread because I don't see tests that cross the day boundary. If the hour is 10, what happens when the offset is -11:00? Or if it's 15 and the offset is 10:00?

zslayton

🚀

Couple of small things left before merging.

zslayton · 2023-03-17T22:47:14Z

src/types/timestamp.rs

@@ -159,9 +159,6 @@ pub struct Timestamp {
    pub(crate) fractional_seconds: Option<Mantissa>,
 }

-// TODO: Timestamp does not yet provide useful accessors for its individual fields. It can be
-//       instantiated and tested for equality, but will not very useful as a general purpose
-//       datetime until these methods are added.


src/types/timestamp.rs

adds accessors for Timestamp

01517e6

* adds year, month, day, hour, minute and second methods for given Timetsamp in local time. * adds utc_hour, utc_minute and utc_second methods for given Timestamp in UTC time. * adds unit tests for these accessor methods.

desaikd requested a review from zslayton March 14, 2023 21:31

zslayton reviewed Mar 15, 2023

View reviewed changes

popematt reviewed Mar 16, 2023

View reviewed changes

Merge branch 'main' of github.com:amzn/ion-rust into desaikd-timestam…

52ddcd1

…p-accessor

desaikd mentioned this pull request Mar 17, 2023

add Timestamp#to_utc() to return UTC timestamp #489

Closed

adds suggested changes

249db37

jobarr-amzn requested changes Mar 17, 2023

View reviewed changes

src/types/timestamp.rs Outdated Show resolved Hide resolved

popematt reviewed Mar 17, 2023

View reviewed changes

src/types/timestamp.rs Outdated Show resolved Hide resolved

zslayton requested changes Mar 17, 2023

View reviewed changes

adds more tests and doc comment changes

1e8f911

zslayton approved these changes Mar 17, 2023

View reviewed changes

desaikd and others added 2 commits March 17, 2023 16:09

add more suggested changes

8e6c891

Merge branch 'main' into desaikd-timestamp-accessor

f0b9ba7

jobarr-amzn approved these changes Mar 17, 2023

View reviewed changes

desaikd merged commit b78bb71 into main Mar 17, 2023

jobarr-amzn deleted the desaikd-timestamp-accessor branch March 17, 2023 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds accessor methods for `Timestamp` #482

adds accessor methods for `Timestamp` #482

desaikd commented Mar 14, 2023 •

edited

codecov bot commented Mar 14, 2023 •

edited

zslayton Mar 15, 2023

desaikd Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 15, 2023

zslayton Mar 17, 2023

jpschorr commented Mar 15, 2023 •

edited

popematt Mar 16, 2023

zslayton Mar 16, 2023

desaikd Mar 17, 2023 •

edited

popematt left a comment

zslayton Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 17, 2023

zslayton left a comment

zslayton Mar 17, 2023

adds accessor methods for Timestamp #482

adds accessor methods for Timestamp #482

Conversation

desaikd commented Mar 14, 2023 • edited

codecov bot commented Mar 14, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpschorr commented Mar 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desaikd Mar 17, 2023 • edited

Choose a reason for hiding this comment

popematt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adds accessor methods for `Timestamp` #482

adds accessor methods for `Timestamp` #482

desaikd commented Mar 14, 2023 •

edited

codecov bot commented Mar 14, 2023 •

edited

jpschorr commented Mar 15, 2023 •

edited

desaikd Mar 17, 2023 •

edited