search

unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.
https://icu4x.unicode.org
Other
1.38k stars 178 forks source link

Change time fields to time precision option #5776

Closed sffc closed 2 weeks ago

sffc commented 2 weeks ago

1317

sffc commented 2 weeks ago

Change in data size:

datetime/patterns/time_skeleton@1, <lookup>, 4317B, 920 identifiers
datetime/patterns/time_skeleton@1, <total>, 5292B, 2412B, 96 unique payloads

becomes

datetime/patterns/time_skeleton@1, <lookup>, 2762B, 457 identifiers
datetime/patterns/time_skeleton@1, <total>, 5118B, 3318B, 63 unique payloads

The smaller lookup table should compensate for the slightly regression in postcard size. But all these numbers are in the low kB so I'm not concerned one way or another.

robertbastian commented 2 weeks ago

Can you post the diff for <calendar>/skeleton@1?

sffc commented 2 weeks ago

Can you post the diff for <calendar>/skeleton@1?

On main:

datetime/patterns/buddhist/skeleton@1, <lookup>, 9229B, 1902 identifiers
datetime/patterns/buddhist/skeleton@1, <total>, 87610B, 62957B, 824 unique payloads
...
datetime/patterns/gregory/skeleton@1, <lookup>, 10280B, 2138 identifiers
datetime/patterns/gregory/skeleton@1, <total>, 133379B, 105806B, 970 unique payloads

On the branch:

datetime/patterns/buddhist/skeleton@1, <lookup>, 8762B, 1775 identifiers
datetime/patterns/buddhist/skeleton@1, <total>, 91938B, 67289B, 828 unique payloads
...
datetime/patterns/gregory/skeleton@1, <lookup>, 9703B, 1986 identifiers
datetime/patterns/gregory/skeleton@1, <total>, 137763B, 110455B, 966 unique payloads
sffc commented 2 weeks ago

There are fewer identifiers because Ehm and Ehms got merged.

The payloads are bigger because there is slightly less deduplication and because Eh data was added to it.

I don't like overlap patterns.

robertbastian commented 2 weeks ago

Thanks.

I don't like overlap patterns.

I don't know what that means.

Manishearth commented 2 weeks ago

I give up on posting PR comments here, it's too slow.

I ended up reviewing this PR in a git diff.

Main comments are that I don't quite like the name HourPlus (yes, it was my idea) but I don't have anything better. HourExact sounds fine, HourOnly would also work.

sffc commented 2 weeks ago

This PR should be reviewable commit-by-commit. I did not do things in earlier commits that I undid in later commits.

sffc commented 2 weeks ago

Remarkably there wasn't any major merge conflict between this and @robertbastian's datetime PRs. Only data that needed to be re-generated.

sffc commented 2 weeks ago

Is there anything blocking this from landing?

robertbastian commented 2 weeks ago

lgtm