Add the AM/PM getters on FixedCalendarDateTimeNames #7127
Conversation
|
|
|
Thank you for the contribution! I'll leave comments soon. In the meantime I triggered the continuous integration. |
83b643e to
4259cb6
Compare
|
@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. |
Understood, thanks for the guidance! |
| /// None | ||
| /// ); | ||
| /// ``` | ||
| pub fn get_month(&self, month_code: MonthCode, length: MonthNameLength) -> Option<&str> { |
There was a problem hiding this comment.
issue: MonthCode is probably not the type we want to use here. #7149
There was a problem hiding this comment.
Note: We want to accept a "formatting" month, which differentiates between Adar and Adar II.
| /// None | ||
| /// ); | ||
| /// ``` | ||
| pub fn get_era(&self, era_year: &EraYear, length: YearNameLength) -> Option<&str> { |
There was a problem hiding this comment.
issue: clients won't know how to construct an EraYear correctly, so this method is only useful for eras returned by a Date.
do we want to add support for accept era codes (like ce) here? we would need some kind of integration with icu::calendar.
1a28516 to
a0fd595
Compare
|
Thank you @robertbastian for the review and feedback! I completely understand both concerns you raised:
Therefore, I've decided to keep only I'll try to implement the other getters after these issues are resolved:
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! |
related issue #6603