icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

Map 'B' pattern item to NoonMidnight

Open nordzilla opened this issue 4 years ago • 5 comments

Depends on #1216

  • Updates the field_type macro to allow multiple characters to map to the same key.
  • Updates the field_type macro to support optional trailing commas.
  • Maps both 'b' and 'B' to NoonMidnight.
  • Updates failing test for zh-Hant which uses 'B' for medium lenght time format.

@sffc

It's unclear to me why we're allowing this change when it seems to be incorrect behavior in the general case. The 'B' item should be flexible day periods which aren't implemented yet, to my knowledge.

While this PR fixes the failing test for zh-Hant, I feel like it would introduce incorrect behavior in general by mapping 'B' to NoonMidnight.

This also introduces awkward behavior where char::try_from::<DayPeriod>() can only choose one of the characters. I decided to leave NoonMidnight mapping to 'b'.

This would break round-tripping for char::try_from(DayPeriod::try_from('B').unwrap()), which would produce 'b'.

nordzilla avatar Oct 28 '21 01:10 nordzilla

@macchiati @pedberg-icu @srl295

If we don't support 'B' yet, is it reasonable to replace it with 'b' ?

sffc avatar Oct 28 '21 01:10 sffc

:tada: All dependencies have been resolved !

dpulls[bot] avatar Oct 29 '21 20:10 dpulls[bot]

The best replacement would be to look though the <timeData>. The allowed attribute should be used. Go through the list until you get to the first one you support. Eg:

	<hours preferred="h" allowed="hB hb h H" regions="TW ET gu_IN mr_IN pa_IN"/>

NOTE: The preferred attribute is just for backwards compatibility.

macchiati avatar Oct 29 '21 21:10 macchiati

Discussion: put this PR in a dormant state and revisit before 0.5 if the real issue to support B has not yet been fixed (#487)

sffc avatar Nov 05 '21 17:11 sffc

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 01 '22 06:09 CLAassistant

@nordzilla Do you have a status update on this PR?

sffc avatar Nov 10 '22 18:11 sffc