Point out that AstronomicalSimulation exists for semver and users who are unsure probably want UmmAlQura instead #7031
Conversation
who are unsure probably want UmmAlQura instead. Fixes unicode-org#6210
Using Gemini Code AssistThe 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
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 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. |
| /// | ||
| /// These simulations are known to not necessarily match sightings on the ground, | ||
| /// but are included for completeness. | ||
| /// and are included for semver reasons rather than use case reasons. Unless you know |
There was a problem hiding this comment.
if we're saying they're included only for semver then that means we would prefer deleting them but can't. do we? they should be deprecated then
There was a problem hiding this comment.
My understanding is that we don't have a use case-motivated reason to ship this, which indeed makes it strange that it's not deprecated. @sffc can you confirm that my understanding and that this should be marked as deprecated?
There was a problem hiding this comment.
can you remove the semver justification until we have that discussion?
There was a problem hiding this comment.
I don't think it should be deprecated.
I think if we were to add it today we would have a use case motivated discussion but I do not think that means we should remove it.
We have it now, it's not a bad calendar, it's fine.
There was a problem hiding this comment.
I also prefer a weaker statement here without focusing on "semver"
There was a problem hiding this comment.
We've discussed previously how we have a fairly low bar for accepting calendars into icu_calendar, but a high bar for accepting them into AnyCalendar. It's a different standard in my mind than other components. We've specifically designed icu_calendar to be able to handle a long tail of calendars.
There was a problem hiding this comment.
That said I think the bar for icu_calendar does include having a good idea of users because without that it's easy to get it wrong (e.g. we spent a fair amount of time trying to understand Coptic/ethiopic era directions). Its possible this calendar doesn't reach that bar, but overall I am comfortable including it since it's based on a book that documents it well.
| /// These simulations are known to not necessarily match sightings on the ground, | ||
| /// but are included for completeness. | ||
| /// and are included for semver reasons rather than use case reasons. Unless you know | ||
| /// otherwise for sure, instead of this variant, use `UmmAlQura`, which uses the results |
There was a problem hiding this comment.
use link
| /// otherwise for sure, instead of this variant, use `UmmAlQura`, which uses the results | |
| /// otherwise for sure, instead of this variant, use [`UmmAlQura`], which uses the results |
| /// Creates a [`Hijri`] calendar using simulated sightings at Mecca. The | ||
| /// simulation in not official and this variant is provided for semver rather | ||
| /// than use case reasons. Unless you know otherwise for sure, instead of | ||
| /// this variant, use `Hijri::new_umm_al_qura`, which uses the results of |
There was a problem hiding this comment.
| /// this variant, use `Hijri::new_umm_al_qura`, which uses the results of | |
| /// this variant, use [`Hijri::new_umm_al_qura`], which uses the results of |
Fixes #6210