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

DurationFormatter: Digital Data Provider SourceDataProvider impl #5208

Merged

Conversation

kartva
Copy link
Member

@kartva kartva commented Jul 9, 2024

Will add bakeddata and testdata in separate PR.

Follow up to #5186.

@kartva kartva requested a review from a team as a code owner July 9, 2024 14:55
@kartva kartva requested a review from younies July 9, 2024 15:06
Copy link
Member

@younies younies left a comment

Choose a reason for hiding this comment

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

Good job, add the result of cargo make testdata

@kartva
Copy link
Member Author

kartva commented Jul 14, 2024

Note that es-CL has inconsistent hour padding for hm and hms:

"durationUnit-type-hm": {
  "durationUnitPattern": "h:mm"
},
"durationUnit-type-hms": {
  "durationUnitPattern": "hh:mm:ss"
},

@kartva kartva force-pushed the duration-formatter-digital-provider-impl branch from 3ff08a8 to 49c508b Compare July 14, 2024 02:51
@kartva kartva requested a review from younies July 14, 2024 03:43
Copy link
Member

@younies younies left a comment

Choose a reason for hiding this comment

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

Good code.

@kartva kartva requested review from younies and sffc July 20, 2024 01:18
"m": 1,
"s": 1
}
}
Copy link
Member

Choose a reason for hiding this comment

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

this can be optimized, but we can do it in another PR: @kartva , just file a but to optimize this.

Copy link
Member Author

@kartva kartva Jul 22, 2024

Choose a reason for hiding this comment

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

To clarify, you are recommending that I file an issue about optimizing this?

Copy link
Member

Choose a reason for hiding this comment

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

I think @younies intended to say "file a bug", so yes. Alternatively, if you already have an issue where you are tracking follow-ups, you can add it there.

Copy link
Member

@sffc sffc Jul 23, 2024

Choose a reason for hiding this comment

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

This should be easy to bitpack: I think the only possible values are

pub enum HmsPadding {
    None,
    Hour,
    Minute,
    Second,
    HourMinute,
    HourSecond,
    HourMinuteSecond,
}

Are there any locales with patterns such as the following? If not, we can cut down the options further:

  • h:m:ss
  • h:mm:s
  • h:m:s
  • m:s

Then I think the only options are

pub enum HmsPadding {
    None,
    /// Pad the hours in hh:mm and hh::mm:ss; pad the minutes in mm:ss
    LargestUnit,
}

@younies younies merged commit e2a03b2 into unicode-org:main Jul 21, 2024
28 checks passed
sffc pushed a commit that referenced this pull request Jul 23, 2024
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.

3 participants