Implement Ethopian in terms of Coptic #6952

robertbastian · 2025-09-17T16:19:22Z

These are the same calendar, just using different eras.

This is the same situation as Gregorian, Iso, Buddhist, Japanese, and Roc being the same calendar using different eras.

We might want to make these calendars generic in the future, and have something like Coptic<CopticEra>, Coptic<EthiopianEraStyle>, Gregorian<JapaneseEras>, Gregorian<Iso>, etc.

gemini-code-assist · 2025-09-17T16:19:25Z

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

sffc · 2025-09-17T16:24:33Z

components/calendar/src/cal/gregorian.rs

-    type DateInner = GregorianDateInner;
+    type DateInner = IsoDateInner;


Issue: I think we have the GregorianDateInner type because it's public API and we don't want to lock into a specific representation

it's unstable public API

Buddhist already uses IsoDateInner without wrapping

Associated types are not unstable; you can get DateInners from various places and you might have saved them in a variable. I know I do that. I imagine temporal_rs or some other client who integrates deeply with icu_calendar might do that.

If Buddhist doesn't have a newtype, that's a bug

associated types are obtained only from the Calendar trait, which is unstable

It's Sealed, not unstable. We can't just go and change the signatures of functions on the trait. It is sealed so we can add new functions.

UnstableSealed is the same as Sealed except it means people can still implement it if they are okay with the trait changing. It's about implementers, not users

fact is we document this as being allowed to break semver

icu::calendar::Calendar is a trait that is sealed, and it is also a trait that people can construct and call methods from. It is implemented by a dozen public types in the crate. This is not controversial.

The intent of that documentation was that this trait is unstable to implement. The trait implementors are not going to have the same banner.

We should maybe fix the banner to be clearer about calling the trait, but at the moment a person looking at the docs for an individual calendar will not know that the underlying types are stable.

So this is not unstable public API, and I don't think that's a matter that we should continue to debate.

Now, that said, I am somewhat okay with minor breaking stuff around this provided we do not remove any names from the crate (we add type aliases, like we did in Dangi). We probably should have started with type aliases from the get-go, or started with individual newtypes, to give us freedom to change things.

So if we retain a deprecated GregorianDateInner type alias, that's great.

sffc · 2025-09-17T16:25:15Z

components/calendar/src/cal/ethiopian.rs

 impl crate::cal::scaffold::UnstableSealed for Ethiopian {}
 impl Calendar for Ethiopian {
-    type DateInner = EthiopianDateInner;
+    type DateInner = CopticDateInner;


Issue: This should stay as EthiopianDateInner but that type could change to be a newtype over CopticDateInner

sffc · 2025-09-17T16:31:27Z

components/calendar/src/cal/ethiopian.rs

        if era_style == EthiopianEraStyle::AmeteAlem {
            year -= INCARNATION_OFFSET;
        }


Issue: I was also looking at this code last week, and I think this operation seems wrong. I think it should be

Suggested change

if era_style == EthiopianEraStyle::AmeteAlem {

year -= INCARNATION_OFFSET;

}

if era_style == EthiopianEraStyle::AmeteMihret {

year += INCARNATION_OFFSET;

}

because it seems like the year was stored in Amete Alem for both era styles. In your case, you still need this change if you are trying to store the year in Anno Martyrum.

this seems unrelated to this PR

I guess it's fine if the tests still pass

this code path doesn't seem to be covered by tests. however I'm not touching the alem vs mihret logic here, so it should be fixed and tested in a different PR

This is something we should fix in the from_fields PR, really, or separately.

sffc · 2025-09-17T16:33:39Z

components/calendar/src/cal/roc.rs

-    type DateInner = RocDateInner;
+    type DateInner = IsoDateInner;


Issue: Keep the newtype

sffc

You didn't address my feedback

sffc

.

sffc · 2025-09-17T16:55:36Z

Plumbing Ethiopian to wrap Coptic is a fine change if you wanted to make it. My issue was only with changing the public associated type DateInner.

sffc · 2025-09-17T17:09:44Z

Technically I think adding a type alias would also suffice from a semver perspective, but I still strongly prefer the newtypes. (Not just here but also ideally in Chinese and Hijri)

Manishearth · 2025-09-17T17:19:53Z

Technically I think adding a type alias would also suffice from a semver perspective

It's technically not fully good from a semver perspective but the semver risk is very slight: it only matters for people doing impl MyTrait for Type1 and impl MyTrait for Type2 because now you have clashing impls.

But as with the Chinese/Hijri stuff, I'm okay with this level of breakage. We should not remove names from the crate; so we should introduce deprecated aliases.

sffc

Also, let's wait to merge this until I merge Date::from_fields. I've already needed to rebase it on top of Hijri and Chinese, which was fine because those PRs blocked other work, but this PR strikes me as one that could wait a couple of days and be harmless, and it's small enough that it can be rebased on top of Date::from_fields.

sffc · 2025-09-18T16:39:27Z

components/calendar/src/cal/ethiopian.rs

-/// The inner date type used for representing [`Date`]s of [`Ethiopian`]. See [`Date`] and [`Ethiopian`] for more details.
-///
-/// The year is stored as Amete Alem year
+#[allow(missing_docs)] // not actually public


Nit: Let's not add #[missing_docs]. I would like this type to be public but have no methods on it.

It might be a bug that this type isn't exported, since it can be accessed by Coptic::DateInner.

I'm pretty sure it's intended that these types are only nameable as associated types. If they don't have a public API, why would they need docs.

I think a very short line of docs is good here. We don't need more.

Yes, these are mostly assoc types, but people might stumble upon them via search. We should probably improve the docs for that case (the current docs aren't great!), but we shouldn't remove them.

the current docs aren't great!

the current docs are not public, because this is not a public type

ah, huh. That's fine then

Surprised the lint fires.

Manishearth

r+ on approach, would also prefer from_fields to land first.

Manishearth · 2025-09-18T21:23:14Z

components/calendar/src/cal/ethiopian.rs

-/// The inner date type used for representing [`Date`]s of [`Ethiopian`]. See [`Date`] and [`Ethiopian`] for more details.
-///
-/// The year is stored as Amete Alem year
+#[allow(missing_docs)] // not actually public


I think a very short line of docs is good here. We don't need more.

Yes, these are mostly assoc types, but people might stumble upon them via search. We should probably improve the docs for that case (the current docs aren't great!), but we shouldn't remove them.

Manishearth · 2025-09-18T21:24:35Z

components/calendar/src/cal/ethiopian.rs

        if era_style == EthiopianEraStyle::AmeteAlem {
            year -= INCARNATION_OFFSET;
        }


This is something we should fix in the from_fields PR, really, or separately.

sffc · 2025-09-19T06:05:56Z

One other thing is missing from the PR: why?

For Hijri, there are a lot of different sightings, and the Hijri trait offers convenient functionality utilizing the YearInfo, like arithmetic, ISO conversion, etc., which would be hard to replicate in userland. Same with Chinese.

But I don't see the same motivation applying to ISO-based calendars. You don't gain any functionality by plugging in your own epoch. Do you?

robertbastian · 2025-09-19T13:53:29Z

This deduplicates code that the compiler will not necessarily know to deduplicated (because the diffs are at the very bottom, in calendrical_calculations).

Storing the Ethiopian year as a Coptic year is also way clearer wrt which era is used internally.

robertbastian requested review from Manishearth and sffc as code owners September 17, 2025 16:19

sffc requested changes Sep 17, 2025

View reviewed changes

robertbastian requested a review from sffc September 17, 2025 16:37

sffc requested changes Sep 17, 2025

View reviewed changes

robertbastian requested a review from sffc September 17, 2025 16:41

sffc requested changes Sep 17, 2025

View reviewed changes

robertbastian requested a review from sffc September 17, 2025 16:49

robertbastian closed this Sep 17, 2025

robertbastian deleted the arithmetic branch September 17, 2025 16:53

robertbastian restored the arithmetic branch September 18, 2025 15:04

robertbastian reopened this Sep 18, 2025

implement Ethiopian in terms of coptic

fa7a90c

robertbastian force-pushed the arithmetic branch from 5c00fdc to fa7a90c Compare September 18, 2025 15:05

yay more indirection

e09e09b

sffc requested changes Sep 18, 2025

View reviewed changes

Manishearth previously approved these changes Sep 18, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into arithmetic

c316bcd

robertbastian dismissed Manishearth’s stale review via c316bcd September 19, 2025 13:51

		type DateInner = GregorianDateInner;
		type DateInner = IsoDateInner;

		type DateInner = RocDateInner;
		type DateInner = IsoDateInner;

Implement Ethopian in terms of Coptic #6952

Are you sure you want to change the base?

Implement Ethopian in terms of Coptic #6952

Uh oh!

Conversation

robertbastian commented Sep 17, 2025

Uh oh!

gemini-code-assist bot commented Sep 17, 2025

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 Sep 17, 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!

Manishearth Sep 17, 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!

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!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Sep 17, 2025

Uh oh!

sffc commented Sep 17, 2025

Uh oh!

Manishearth commented Sep 17, 2025

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!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sffc Sep 17, 2025 •

edited

Loading

Manishearth Sep 17, 2025 •

edited

Loading