Support ohos tzdata since ver.oh35 #1613
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1613 +/- ##
=======================================
Coverage 91.11% 91.11%
=======================================
Files 37 37
Lines 17104 17110 +6
=======================================
+ Hits 15584 15590 +6
Misses 1520 1520 ☔ View full report in Codecov by Sentry. |
src/offset/local/tz_info/timezone.rs
Outdated
|
|
||
| #[cfg(target_env = "ohos")] | ||
| fn from_tzdata_file(file: &mut File, tz_string: &str) -> Result<Vec<u8>, Error> { | ||
| let mut bytes: Vec<u8> = Vec::new(); |
There was a problem hiding this comment.
Nit: please ditch the unnecessary type annotation.
src/offset/local/tz_info/timezone.rs
Outdated
| @@ -629,6 +640,63 @@ fn find_tz_file(path: impl AsRef<Path>) -> Result<File, Error> { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(target_env = "ohos")] | |||
| const TZDATA_PATH: &str = "/system/etc/zoneinfo/tzdata"; | |||
There was a problem hiding this comment.
Let's inline this into find_ohos_tz_data().
| @@ -50,6 +53,14 @@ impl TimeZone { | |||
| } | |||
| } | |||
|
|
|||
| // ohos merge all file into tzdata since ver35 | |||
| #[cfg(target_env = "ohos")] | |||
There was a problem hiding this comment.
Is there any point in falling through after this? If so, please add a comment making explicit why boths are useful.
src/offset/local/tz_info/timezone.rs
Outdated
| if let Ok(mut file) = File::open(TZDATA_PATH) { | ||
| return from_tzdata_file(&mut file, tz_string); | ||
| } | ||
| Err(Error::Io(io::ErrorKind::NotFound.into())) |
There was a problem hiding this comment.
There doesn't seem to be much point in synthesizing an io::Error here if we can just use the one from File::open().
src/offset/local/tz_info/timezone.rs
Outdated
| let offset = cursor.read_be_u32()?; | ||
| let length = cursor.read_be_u32()?; | ||
| let zone_name = str::from_utf8(index_buf)?.trim_end_matches('\0'); | ||
| if zone_name == tz_string { |
There was a problem hiding this comment.
Please invert this condition to reduce rightward drift in the block below.
src/offset/local/tz_info/timezone.rs
Outdated
| let _ = cursor.read_be_u32()?; | ||
|
|
||
| if cursor.seek_after(index_offset_offset as usize).is_err() { | ||
| return Err(Error::Io(io::ErrorKind::NotFound.into())); |
There was a problem hiding this comment.
Let's yield the error from seek_after() directly?
|
@djc Thanks for prompt and detailed review. I'm quite new to rust due to my work adjustment. I have modified the codes according to your advice, pls check again, thx! |
| const TZDATA_PATH: &str = "/system/etc/zoneinfo/tzdata"; | ||
| match File::open(TZDATA_PATH) { | ||
| Ok(mut file) => from_tzdata_file(&mut file, tz_string), | ||
| Err(_err) => Err(Error::InvalidTzFile("invalid ohos tz file")), |
There was a problem hiding this comment.
This seems wrong -- why aren't we returning _err here? The error here is not because the tz file is invalid but because we had trouble opening it.
| }; | ||
| } | ||
|
|
||
| Err(Error::Io(io::ErrorKind::NotFound.into())) |
There was a problem hiding this comment.
io::ErrorKind::NotFound is about not being able to find a file, which is not the case here. Let's do a more appropriate error value?
This is to support openHarmony OS(ohos) format timezone data. ohos has merged timezone files into single file 'tzdata' since recent version. pls review this, thanks.