Skip to content

Fix OpenHarmony's tzdata parsing - #1679

Merged
djc merged 1 commit into
chronotope:mainfrom
ldm0:ohos_fix
Jul 8, 2025
Merged

djc merged 1 commit into
chronotope:mainfrom
ldm0:ohos_fix

Conversation

@ldm0

@ldm0 ldm0 commented Mar 20, 2025 •

Copy link
Copy Markdown
Contributor

OpenHarmony's tzdata parsing supported was introduced in #1613.

OpenHarmony's tzdata shares the same characters with the ones on Android, except that each timezone index is 4 byte's smaller(the legacy rawUtcOffset was removed).

OpenHarmony's tzdata
Android's tzdata

As their parsing logic are pretty similar, I unified their logic into src/offset/local/tzdata.rs(I actually created a crate for that, but the logic is simple enough to put into chrono). This also allows us to remove the android_tzdata dependency.

BTW, OpenHarmony's tzdata parsing logic was also added in fallback_timezone, that's because OpenHarmony doesn't have /etc/localtime, people need to use it's time service api to get the timezone name. I created a PR for iana-time-zone to support OpenHarmony's timezone name fetching: strawlab/iana-time-zone#150. After getting the timezone name from iana_time_zone::get_timezone, chrono need to find it's timezone information in the tzdata.

@codecov

codecov Bot commented Mar 20, 2025 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.68%. Comparing base (9245a8f) to head (4ddf8c8).
⚠️ Report is 46 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1679      +/-   ##
==========================================
+ Coverage   90.61%   90.68%   +0.07%     
==========================================
  Files          38       39       +1     
  Lines       16055    16180     +125     
==========================================
+ Hits        14548    14673     +125     
  Misses       1507     1507              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@djc

djc commented Mar 20, 2025

Copy link
Copy Markdown
Member

This should come with CI coverage for Android and OpenHarmony. The new parser should be cleaned up, have a look at the rustls style guide (which I wrote) and try to simplify (drop the OS constructor, make the constant names more concise, avoid nested types/functions).

@ldm0

ldm0 commented Mar 21, 2025

Copy link
Copy Markdown
Contributor Author

@djc Hi, all new lines are covered now, and parser was cleaned up. Please take a look.

Comment thread src/offset/local/tz_info/timezone.rs Outdated
Comment thread src/offset/local/tzdata.rs Outdated
Comment thread src/offset/local/tzdata.rs Outdated
Comment thread src/offset/local/tzdata.rs Outdated
Comment thread src/offset/local/tzdata.rs Outdated
Comment thread src/offset/local/tzdata.rs Outdated
Comment thread src/offset/local/tzdata.rs Outdated
Comment thread src/offset/local/tzdata.rs
Comment thread src/offset/local/tzdata.rs Outdated
Comment thread src/offset/local/tzdata.rs Outdated
@ldm0

ldm0 commented May 27, 2025

Copy link
Copy Markdown
Contributor Author

ping @djc plz take a look

@djc djc left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for working on this.

Can you rebase it and squash your commits, please?

@ldm0

ldm0 commented Jul 8, 2025

Copy link
Copy Markdown
Contributor Author

@djc done

@djc
djc merged commit 23c132f into chronotope:main Jul 8, 2025
@ldm0
ldm0 deleted the ohos_fix branch July 8, 2025 15:02

This branch was previously deployed

1 inactive deployment
Coverage — 4ddf8c82 Deployed Jul 8, 2025 by ldm0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants