Skip to content
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

Using SinglePlaceholderPattern in relative time #5384

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Aug 16, 2024

Warmup for #5256

"index": 7
}
"zero": [
{
Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't really human-readable imho, I'd prefer if Pattern serialized to a pattern string instead.

Copy link
Member

Choose a reason for hiding this comment

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

I would too, but that required adding extra trait functions to PatternBackend whereas serializing a list of PatternItems didn't require adding anything to the trait. I made the judgement when writing icu_pattern that the code cost of supporting nicer human-readable serialization was not worth the benefit.

Copy link
Member

Choose a reason for hiding this comment

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

An easier alternative would be to clean up the serialization so long as it is still a Vec<PatternItem>. So for example, instead of

    [
      {
        "Placeholder": "Singleton"
      },
      {
        "Literal": " yıl önce"
      }
    ]

you could have

    [0, " yıl önce"]

which works if the placeholders are numeric. If the placeholders are strings, maybe something like

    [{ "placeholder": "foo" }, " yıl önce"]

Copy link
Member Author

@robertbastian robertbastian Aug 19, 2024

Choose a reason for hiding this comment

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

0 could serialize to "{0}", and { "placeholder": "foo" } could serialize to "{foo}".

Copy link
Member

Choose a reason for hiding this comment

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

Problem with serializing to "{0}" is that you can't tell if that's a placeholder or the literal string, which means we have to deal with escaping, which is a whole bunch of complexity. We should encode whether the segment is a placeholder or a string in the JSON schema.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Nice

relativetime/long/day@1, zh-Hant, 495B, 84a17fe2c935fbd4
relativetime/long/day@1, zh-MO, 495B, -> zh-HK
relativetime/long/day@1, zu, 632B, 5bb93015fdeca04a
relativetime/long/day@1, <total>, 67791B, 148 unique payloads
Copy link
Member

Choose a reason for hiding this comment

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

Praise: Big size decrease

@robertbastian robertbastian merged commit b50e295 into unicode-org:main Aug 16, 2024
28 checks passed
@robertbastian robertbastian deleted the relative branch August 19, 2024 08:52
@robertbastian
Copy link
Member Author

#5369

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