-
Notifications
You must be signed in to change notification settings - Fork 519
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
Support ohos tzdata since ver.oh35 #1613
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1613 +/- ##
==========================================
+ Coverage 91.11% 91.12% +0.01%
==========================================
Files 37 37
Lines 17104 17129 +25
==========================================
+ Hits 15584 15609 +25
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
src/offset/local/tz_info/timezone.rs
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/offset/local/tz_info/timezone.rs
Outdated
}; | ||
} | ||
|
||
Err(Error::Io(io::ErrorKind::NotFound.into())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed this to "InvalidTzString" error, because cannot find tzdata chunk via tz string
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.