icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

Add the AM/PM getters on FixedCalendarDateTimeNames

Open WaterWhisperer opened this issue 2 months ago • 9 comments

related issue #6603

WaterWhisperer avatar Oct 19 '25 13:10 WaterWhisperer

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 19 '25 13:10 CLAassistant

Thank you for the contribution! I'll leave comments soon. In the meantime I triggered the continuous integration.

sffc avatar Oct 19 '25 17:10 sffc

@sffc Thanks for the review!

WaterWhisperer avatar Oct 20 '25 05:10 WaterWhisperer

Are we fine making this stable?

this PR does not fix the issue as it only exposes AM/PM, no month or day names

robertbastian avatar Oct 21 '25 16:10 robertbastian

I'm okay with this API as written. It doesn't seem like it fixes the whole issue though, so I'm wondering if we should wait to merge this until after 2.1.

Manishearth avatar Oct 21 '25 18:10 Manishearth

Thanks for the reviews!

I see the discussion about this not fully addressing #6603. The original issue includes getters for weekdays, month names, and era codes as well.

Should I extend this PR to include those, or handle them in separate PRs?

Happy to work on whichever approach is most helpful. Thanks for your patience with a first-time contributor! 😊

WaterWhisperer avatar Oct 22 '25 13:10 WaterWhisperer

@WaterWhisperer Either is fine: the discussion is about merging this before ICU4X 2.1, releasing in the next few weeks. Once that release happens we're more open to merging this as-is.

I think having all of the methods in the same PR is a good idea anyway. We already have alignment on the general approach.

Manishearth avatar Oct 22 '25 15:10 Manishearth

@WaterWhisperer Either is fine: the discussion is about merging this before ICU4X 2.1, releasing in the next few weeks. Once that release happens we're more open to merging this as-is.

I think having all of the methods in the same PR is a good idea anyway. We already have alignment on the general approach.

Understood, thanks for the guidance!

WaterWhisperer avatar Oct 23 '25 04:10 WaterWhisperer

Thank you @robertbastian for the review and feedback!

I completely understand both concerns you raised:

  1. MonthCode issue: I see issue #7149, and I agree that we need a validated Month type. The current MonthCode requires handling invalid syntax in every API.

  2. EraYear issue: You're right that clients won't know how to properly construct an EraYear. Using era code strings (like "ce") would be much more user-friendly.

Therefore, I've decided to keep only get_am and get_pm getters in this PR. I've reverted the other getters (weekday, month, era) and rebased the branch onto the latest main.

I'll try to implement the other getters after these issues are resolved:

  • get_month: waiting for #7149
  • get_era: needs API redesign to accept era code strings
  • get_weekday: the API looks fine, but to keep this PR focused, I'll save it for later

This way, this PR focuses on the most basic AM/PM getter functionality from issue #6603, and the remaining getters can be improved incrementally in future PRs.

Please let me know if this approach works for you!

WaterWhisperer avatar Oct 23 '25 15:10 WaterWhisperer