Add the AM/PM getters on FixedCalendarDateTimeNames
related issue #6603
Thank you for the contribution! I'll leave comments soon. In the meantime I triggered the continuous integration.
@sffc Thanks for the review!
Are we fine making this stable?
this PR does not fix the issue as it only exposes AM/PM, no month or day names
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.
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 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.
@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!
Thank you @robertbastian for the review and feedback!
I completely understand both concerns you raised:
-
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.
-
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!