convert metazone period from string to i32 #2085
Conversation
samchen61661
requested review from
a team,
sffc,
robertbastian,
Manishearth,
zbraniecki,
gregtatum and
nordzilla
as code owners
samchen61661
removed request for
a team,
zbraniecki,
gregtatum and
robertbastian
|
Hi Sam! Sorry for the delay on this! I am working on my review but I wanted to add @Manishearth to review the icu_calendar part of it. |
| "2020-03-07 16:00": "auwe", | ||
| "2020-10-03 16:01": "case" | ||
| "0": "auwe", | ||
| "20931480": "case", |
There was a problem hiding this comment.
Thought: I wonder if we should serialize the integer in Postcard and the nice string in JSON.
The way to do this would be to make a new type like MinutesSinceEpoch, use it instead of i32 in the ZeroMap, and give it a custom Serialize and Deserialize impl.
I don't want you to do it in this PR, but if you think it's a good idea, you can open an issue and do it as a follow-up.
There was a problem hiding this comment.
What benefits we can get from it apart from type checking and code style?
There was a problem hiding this comment.
Json file would be more readable, json file API customized.
Serialize as raw date timestamp in Json and deserialize as MinutesSinceEpoch.
File an issue for it.
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
| let minutes_a_hour = 60; | ||
| let hours_a_day = 24; | ||
| let minutes_a_day = minutes_a_hour * hours_a_day; | ||
| if let Ok(unix_epoch) = DateTime::new_iso_datetime(1970, 1, 1, 0, 0, 0) { |
There was a problem hiding this comment.
Updated the math since we need to consider time here.
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
| /// assert_eq!(today.minutes_since_local_unix_epoch(), 0); | ||
| /// assert_eq!(DateTime::from_minutes_since_local_unix_epoch(0), Ok(today)); | ||
| /// ``` | ||
| pub fn minutes_since_local_unix_epoch(&self) -> i32 { |
There was a problem hiding this comment.
Thought: We need to think about what happens when overflow occurs, but this is a problem that is all over the icu_calendar crate, not just this function, so we shouldn't block this PR on a resolution to it. I opened #2151
* convert metazone period from string to i32 * address comments * address minor comments * remove unnecssary import * fix test * make testdata
Convert metazone period from string to i32 (#2034)