Skip to content

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented May 5, 2025

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @robertbastian, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request focuses on updating era codes and correcting inconsistencies across several calendar implementations. Specifically, it addresses issues in the Indian, ROC (Republic of China), and Hijri calendars. The changes involve renaming era codes, updating documentation, and correcting the logic for handling years before the common era in the Hijri calendar. The PR also includes updates to tests to reflect these changes and ensure the calendars function as expected.

Highlights

  • Indian Calendar Era Code Update: The era code for the Indian calendar has been changed from saka to shaka to align with the correct spelling. This change is reflected in the code and documentation.
  • ROC Calendar Era Code Update: The era codes for the ROC calendar have been updated from minguo and minguo-qian to roc and broc respectively. This change affects the code, documentation, and tests.
  • Hijri Calendar Era Handling: The Hijri calendar implementation has been updated to support two era codes: ah (Anno Hegirae) for years after the Hijra and bh (Before Hijrah) for years before the Hijra. The logic for calculating the year based on the era has been corrected, and a regression test has been added to ensure correct handling of years in the bh era.

Changelog

Click here to see the changelog
  • components/calendar/src/any_calendar.rs
    • Updated era codes in tests for Indian and ROC calendars to reflect the new naming conventions (shaka instead of saka, roc and broc instead of minguo and minguo-qian).
  • components/calendar/src/cal/hijri.rs
    • Introduced support for two era codes: ah (Anno Hegirae) and bh (Before Hijrah).
    • Updated the era_year function to handle years before the Hijra.
    • Corrected the documentation to reflect the new era codes.
    • Added year_check import from crate::error.
  • components/calendar/src/cal/indian.rs
    • Corrected the spelling of the era code from saka to shaka in the code and documentation.
    • Updated comments to reflect the correct name of the Śaka era.
    • Updated the year_info function to use the correct era code (shaka).
  • components/calendar/src/cal/roc.rs
    • Updated era codes from minguo and minguo-qian to roc and broc respectively.
    • Updated the year_info function to use the new era codes.
    • Updated tests to reflect the new era codes and ensure correct handling of years in both eras.
  • provider/source/src/calendar/eras.rs
    • Updated era code mappings to use shaka for the Indian calendar and roc/broc for the ROC calendar.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue 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 is currently in preview and 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 to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


A calendar's tale,
Eras shift, names prevail,
Time's river flows.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates era codes in the ICU4X calendar components. The changes include renaming era codes for the Indian and ROC calendars and adding support for a 'before Hijrah' era in the Hijri calendar. The code modifications seem correct and address the issue described in the pull request description.

Summary of Findings

  • Correctness of Hijri Calendar Era Handling: The addition of 'bh' era handling in the Hijri calendar implementations seems correct, but it's crucial to ensure that all edge cases related to negative year values are thoroughly tested to avoid potential logical errors.
  • Testing for new era codes: The tests have been updated to reflect the new era codes, but it's important to ensure that the tests cover a wide range of dates, including edge cases and boundary conditions, to ensure the correctness of the calendar implementations.

Merge Readiness

The pull request appears to be well-structured and addresses the intended issue. However, due to the introduction of new era codes and changes in calendar logic, I recommend thorough testing, especially for edge cases and boundary conditions related to negative year values in the Hijri calendar. I am unable to approve this pull request, and it should be reviewed and approved by others before merging. It is recommended that the pull request not be merged until the medium severity issues are addressed.

Comment on lines +386 to +387
Some("ah") | None => year_check(year, 1..)?,
Some("bh") => 1 - year_check(year, 1..)?,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a comment explaining why the year is being checked against 1.. and 1-year respectively. This will help future developers understand the logic behind the code.

            Some("ah") | None => year_check(year, 1..)?, // Years must be positive in AH era
            Some("bh") => 1 - year_check(year, 1..)?, // Years must be positive in BH era

Comment on lines +669 to +670
Some("ah") | None => year_check(year, 1..)?,
Some("bh") => 1 - year_check(year, 1..)?,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a comment explaining why the year is being checked against 1.. and 1-year respectively. This will help future developers understand the logic behind the code.

            Some("ah") | None => year_check(year, 1..)?, // Years must be positive in AH era
            Some("bh") => 1 - year_check(year, 1..)?, // Years must be positive in BH era

Comment on lines +895 to +896
Some("ah") | None => year_check(year, 1..)?,
Some("bh") => 1 - year_check(year, 1..)?,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a comment explaining why the year is being checked against 1.. and 1-year respectively. This will help future developers understand the logic behind the code.

            Some("ah") | None => year_check(year, 1..)?, // Years must be positive in AH era
            Some("bh") => 1 - year_check(year, 1..)?, // Years must be positive in BH era

Comment on lines +70 to +71
Some("roc") | None => ROC_ERA_OFFSET + year_check(year, 1..)?,
Some("broc") => ROC_ERA_OFFSET + 1 - year_check(year, 1..)?,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a comment explaining why the year is being checked against 1.. and 1-year respectively. This will help future developers understand the logic behind the code.

Suggested change
Some("roc") | None => ROC_ERA_OFFSET + year_check(year, 1..)?,
Some("broc") => ROC_ERA_OFFSET + 1 - year_check(year, 1..)?,
Some("roc") | None => ROC_ERA_OFFSET + year_check(year, 1..)?, // Years must be positive in ROC era
Some("broc") => ROC_ERA_OFFSET + 1 - year_check(year, 1..)?, // Years must be positive in BROC era

@Manishearth Manishearth merged commit e4ad164 into unicode-org:main May 5, 2025
29 checks passed
@robertbastian robertbastian deleted the eras branch May 5, 2025 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants