Expand time API #55
Expand time API #55
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Rollczi, 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!
This pull request refactors the "TemporalAmountParser" class to enhance its time formatting capabilities by separating the logic of parsing temporal amounts from their string representation. It introduces new data structures, "TimePart" and "TimeResult", to encapsulate the components of a parsed temporal amount and its sign. This separation of concerns allows for more flexible handling and potential alternative representations of time durations beyond simple string formatting.
Highlights
- Refactor TemporalAmountParser: The existing "format" method in "TemporalAmountParser" has been refactored. Its core logic for calculating time components and determining the sign is now encapsulated in a new "prepare" method, which returns a structured "TimeResult" object. The "format" method now consumes this "TimeResult" to produce the final string.
- Introduce TimePart Record: A new Java record, "TimePart", has been introduced. This record is designed to hold a single component of a temporal amount, such as the numerical count (e.g., 30), its abbreviated name (e.g., "s" for seconds), and the corresponding "ChronoUnit".
- Introduce TimeResult Record: A new Java record, "TimeResult", has been added. This record serves as a container for the output of the "prepare" method, holding a list of "TimePart" objects and a boolean flag indicating whether the original temporal amount was negative.
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 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.
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.
Footnotes
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively expands the time API by separating the parsing logic from the formatting logic in TemporalAmountParser. The introduction of the prepare() method along with TimeResult and TimePart records is a great architectural improvement, making the API more flexible and reusable. My review includes a couple of suggestions: one to remove a redundant check for improved code clarity, and another to add unit tests for the new public prepare method to ensure its correctness and maintainability.
| */ | ||
| public String format(T temporalAmount) { | ||
| StringBuilder builder = new StringBuilder(); | ||
| public TimeResult prepare(T temporalAmount) { |
There was a problem hiding this comment.
The new public method prepare should have its own unit tests. While the existing tests for format will indirectly cover some of prepare's logic, they don't validate the structure of the returned TimeResult object (e.g., the isNegative flag, or the details of each TimePart). Adding dedicated tests for prepare would ensure its contract is met and prevent future regressions.
| for (TimePart part : result.parts()) { | ||
| if (part.count().equals(BigInteger.ZERO)) { | ||
| continue; | ||
| } | ||
|
|
||
| builder.append(part.count()).append(part.name()); | ||
| } |
There was a problem hiding this comment.
This loop contains a redundant check. The prepare() method already filters out time parts with a zero count, so the if (part.count().equals(BigInteger.ZERO)) check is unnecessary. You can simplify the loop.
for (TimePart part : result.parts()) {
builder.append(part.count()).append(part.name());
}
No description provided.