Conversation
Excludes the centisecond, hectosecond, and 100ka. These units have strange names that make them redundant with existing units: "ten milli second", "hundred second", and "100000 years" So say "200 s" would be converted to "2 hundred seconds", and "0.02 s" would be converted to "2 ten milli second". It's also strange that 100ka exists but not 1ka. These units are rarely used, see: https://english.stackexchange.com/questions/79768/what-do-you-call-one-hundredth-of-a-second https://www.reddit.com/r/answers/comments/1p0tc9/why_are_hundredths_of_a_second_commonly_referred/
There was a problem hiding this comment.
Hello @aaronkollasch, 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
Hello! gemini-code-assist here, providing a summary of this pull request to help everyone quickly understand the changes. This PR, titled "Exclude rarely used time units", aims to simplify the unit conversion process by removing support for a few specific time units that are considered rarely used or redundant. Specifically, the units 'centisecond' ('cs'), 'hectosecond' ('hs'), and '100ka' (100,000 years) are being excluded. The rationale provided is that 'cs' is redundant with 'millisecond', 'hs' is redundant with 'second' and less common than 'minute', and '100ka' is strange without '1ka' and redundant with just 'years'. The changes implement this exclusion by adding these units to a blacklist.
Highlights
- Unit Exclusion: Excludes the time units 'centisecond' ('cs'), 'hectosecond' ('hs'), and '100ka' from the supported units by adding them to a blacklist.
- Code Simplification: Removes support for rarely used units to potentially simplify future handling and prevent awkward conversions like 'two hundred seconds' for '200 s' or 'two ten milli second' for '0.02 s', as noted in the PR description.
Changelog
- converter/constants.py
- Added 'cs', 'hs', and '100ka' to the
UNIT_BLACKLISTset (lines 142-144) to exclude these time units from conversion.
- Added 'cs', 'hs', and '100ka' to the
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.
Rare units removed,
Centisecond, hectosecond,
Simplify the code.
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 aims to exclude rarely used time units (cs, hs, 100ka) by adding them to the ANNOTATION_BLACKLIST in converter/constants.py. The justification provided in the PR description for excluding these units—due to redundancy, strange naming, and uncommon usage—is clear and well-supported by the provided links and reasoning. The code change itself is straightforward, minimal, and correctly implements the intended exclusion by leveraging an existing blacklisting mechanism. Overall, this is a well-defined and cleanly executed change.
Summary of Findings
ANNOTATION_BLACKLISTOrder: The newly blacklisted units ('cs','hs','100ka') have been appended to theANNOTATION_BLACKLISTset. The existing items in this set do not follow a strict sorting order. While adding to the end is acceptable and consistent with the current state, for very large lists, adopting a consistent sorting order (e.g., alphabetical) could enhance long-term maintainability. This is a minor stylistic observation of low severity, and the current approach is perfectly fine. I did not add a direct comment as per the review settings, which restrict comments to medium severity or higher.
Merge Readiness
The changes in this pull request are clear, well-justified, and correctly implemented. The code effectively excludes the specified time units as intended. I found no critical, high, or medium severity issues in the provided diff. Based on my review of the code changes, this PR appears to be in good shape for merging. As always, please ensure any necessary testing or further domain-specific review is complete. I am not authorized to approve pull requests, so this feedback is for your consideration and that of other designated reviewers.
Excludes the centisecond, hectosecond, and 100ka.
These units have strange names that make them redundant with existing units:
So say "200 s" would be converted to "2 hundred seconds", and "0.02 s" would be converted to "2 ten milli second".
See also:
https://english.stackexchange.com/questions/79768/what-do-you-call-one-hundredth-of-a-second
https://www.reddit.com/r/answers/comments/1p0tc9/why_are_hundredths_of_a_second_commonly_referred/