Month type #7147

robertbastian · 2025-10-22T21:02:07Z

sffc

Mostly in favor of this, but it shouldn't touch DateFields

sffc · 2025-11-03T23:54:24Z

components/calendar/src/types.rs

    }

+    /// Constructs a non-leap with with the given number.
+    pub const fn new(number: u8) -> Self {


Suggestion: I think I prefer new_common() / new_normal() and new_leap() over new() and new().leap()

My thinking here is that the vast majority of calls will be for non-lunisolar calendars, where the concept of a "normal month" is more confusing than helpful. It's just a month, and you can make it into a leap month if needed

I think new/new_leap is fine, but I'm not really sure what I prefer here.

I'm okay landing this now and bikeshedding it later

components/calendar/src/types.rs

sffc · 2025-11-03T23:56:15Z

components/calendar/src/types.rs

        /// For a full example, see [`Self::ordinal_month`].
-        pub month_code: Option<&'a [u8]>,
-        /// See [`MonthInfo::ordinal`](crate::types::MonthInfo::ordinal).
+        pub month: Option<Month>,


Issue: This should continue to accept a stringy month code.

I will split that discussion into a separate PR I guess

New idea: we add a new field called month, keep month_code, and rename era to era_code.

We already have code to resolve conflicts between month_code and ordinal_month and we can easily throw month into that resolver.

Seems reasonable

Manishearth

I like the effect this has on the tests

Manishearth · 2025-11-06T21:10:25Z

components/calendar/src/types.rs

+    pub(crate) formatting: Month,
+}
+
+impl core::ops::Deref for MonthInfo {


issue: we shouldn't do this, this is considered bad practice for non-pointers.

https://users.rust-lang.org/t/understanding-the-perils-of-deref/47958

hmm, any idea how to not duplicate the is_leap and number methods on both Month and MonthInfo but also not have something like date.month().standard().number()?

I think it's fine to duplicate them.

I agree that we shouldn't require people to call that method chain to get the number

Manishearth · 2025-11-06T21:45:40Z

components/calendar/src/types.rs

        self.standard.is_leap()
    }

+    #[doc(hidden)] // exposed, not a great name


nit: probably mention who the user is. I think icu_datetime.

not even, datetime uses the formatting_ getters. we don't use this anywhere, but we released it in 2.1

I would like to not change a public API into a hidden API in this pull request. This is a perfectly valid public API aimed at clients, not just ICU4X components, that has been public since at least 2.0 and maybe longer.

MonthInfo was new in 2.0. It came initially with three public fields and two functions, month_number being one.

https://docs.rs/icu_calendar/2.0.6/icu_calendar/types/struct.MonthInfo.html#method.month_number

Ah I guess 2.0, I'd prefer deprecating it then. It's just not a great name to have date.month().month_number(), the second month is redundant.

Yeah, please call out public API changes like this, we shouldn't have to figure it out.

I noticed it, assumed from the comment that it was internal, left my comment and decided to come back to it when I had more time after lunch. Probably would have noticed when I looked closer but ideally I don't have to try and second guess everything for public API changes.

semver CI did call this out

sffc

Reviewed down through types.rs

sffc · 2025-11-06T21:56:37Z

components/calendar/src/error.rs

-pub(crate) enum MonthCodeParseError {
+#[non_exhaustive]
+pub enum MonthCodeParseError {
+    /// Invalid syntax


Nit

Suggested change

/// Invalid syntax

/// The month code syntax is invalid.

and please add an example.

sffc · 2025-11-06T21:58:20Z

components/calendar/src/types.rs

-/// A [`MonthCode`] that has been parsed into its internal representation.
-#[derive(Copy, Clone, Debug, PartialEq)]
-pub(crate) struct ValidMonthCode {
+/// A representation of a month.


Nit: Needs much more docs.

sffc · 2025-11-06T22:00:11Z

components/calendar/src/types.rs


-    #[doc(hidden)]
+    #[doc(hidden)] // for formatting
    pub fn formatting_month_number(self) -> u8 {


Suggestion: I thought icu_datetime would just read the formatting month field from the MonthInfo? Do we need these fns?

yeah this removes the fallibility of parsing the code

oh right. Well since this is internal and it is between tilde-dep crates, you should be able to remove these functions if you start publicly exporting the formatting month as a Month.

sffc · 2025-11-06T22:01:54Z

components/calendar/src/types.rs

        self.standard.is_leap()
    }

+    #[doc(hidden)] // exposed, not a great name


I would like to not change a public API into a hidden API in this pull request. This is a perfectly valid public API aimed at clients, not just ICU4X components, that has been public since at least 2.0 and maybe longer.

sffc · 2025-11-06T22:10:23Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new Month type to represent calendar months, replacing the previous MonthCode and ValidMonthCode types. This is a significant and positive refactoring that improves type safety and API clarity. The changes are applied consistently across the calendar component, including core logic, tests, benchmarks, and FFI bindings. My review found one minor issue where a newly hidden API is used in a doc test, which should be updated to use the new public API.

components/calendar/src/ixdtf.rs

robertbastian force-pushed the month branch from 622b1e5 to 7467548 Compare November 3, 2025 13:24

sffc requested changes Nov 3, 2025

View reviewed changes

robertbastian force-pushed the month branch from 7467548 to 53cd4ef Compare November 6, 2025 15:35

robertbastian added 2 commits November 6, 2025 17:15

ValidMonthCode -> Month

4f8616e

use Month in ecma_reference_year methods

817bfa4

robertbastian force-pushed the month branch 2 times, most recently from 754fcdf to bbaf993 Compare November 6, 2025 16:33

robertbastian added 2 commits November 6, 2025 17:54

public API

6ee4db2

update tests

c0b6e78

robertbastian force-pushed the month branch from bbaf993 to c0b6e78 Compare November 6, 2025 16:55

robertbastian marked this pull request as ready for review November 6, 2025 16:56

robertbastian requested review from Manishearth and nekevss as code owners November 6, 2025 16:56

robertbastian requested a review from sffc November 6, 2025 16:56

Manishearth reviewed Nov 6, 2025

View reviewed changes

remove deref

04dd29f

Manishearth reviewed Nov 6, 2025

View reviewed changes

some more tests

f16641f

robertbastian requested a review from Manishearth November 6, 2025 21:59

sffc reviewed Nov 6, 2025

View reviewed changes

sffc mentioned this pull request Nov 6, 2025

Improved changelog procedure #7154

Open

unhide and deprecate

9212d02

gemini-code-assist bot reviewed Nov 6, 2025

View reviewed changes

components/calendar/src/ixdtf.rs Outdated Show resolved Hide resolved

month_number cleanup, formatting getters

66393e6

robertbastian requested a review from zbraniecki as a code owner November 6, 2025 22:18

diplomat

7a039fb

Month type #7147

Are you sure you want to change the base?

Month type #7147

Conversation

robertbastian commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

sffc Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc commented Nov 6, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

robertbastian commented Oct 22, 2025 •

edited

Loading

sffc Nov 3, 2025 •

edited

Loading