Skip to content

Exclude rarely used time units#59

Open
aaronkollasch wants to merge 1 commit intowolph:masterfrom
aaronkollasch:remove-obscure-time-units
Open

Exclude rarely used time units#59
aaronkollasch wants to merge 1 commit intowolph:masterfrom
aaronkollasch:remove-obscure-time-units

Conversation

@aaronkollasch
Copy link
Contributor

Excludes the centisecond, hectosecond, and 100ka.

These units have strange names that make them redundant with existing units:

  • "ten milli second" is redundant with "millisecond"
  • "hundred second" is redundant with "second" and much less common than "minute"
  • "100000 years" is redundant with years, and it's strange that 100ka exists but not 1ka.

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/

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/
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 @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_BLACKLIST set (lines 142-144) to exclude these time units from conversion.
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

  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 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_BLACKLIST Order: The newly blacklisted units ('cs', 'hs', '100ka') have been appended to the ANNOTATION_BLACKLIST set. 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.

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.

1 participant