Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combined inch-foot mentions should be combined into one conversion of distance. #22

Open
Wendelstein7 opened this issue Jun 30, 2021 · 2 comments · May be fixed by #37
Open

Combined inch-foot mentions should be combined into one conversion of distance. #22

Wendelstein7 opened this issue Jun 30, 2021 · 2 comments · May be fixed by #37
Labels
enhancement New feature or request

Comments

@Wendelstein7
Copy link
Owner

Wendelstein7 commented Jun 30, 2021

Currently, message I am 1 inch 3 feet. corrects to I am 2.54 cm 0.914 m..

Rather, it would be favourable if it'd result in one combined SI unit: I am 0.93 m.

@Wendelstein7 Wendelstein7 added the enhancement New feature or request label Jun 30, 2021
@Randall-Scharpf
Copy link
Contributor

To add to your proposition, I know that pounds and ounces (mass units) are used in a similar way, and since I don't hear a number of the units like stones and bushels in colloquial use, I can't say that they aren't used similarly. I think it is reasonable to assume in general that if a unit x and a unit y are of the same type, and y is more precise than x, the phrase A x's and B y's, where A is an integer and B is a non-negative real value, the same treatment should be given as in the case you showed.

Here's some test cases that should work when we're done, whether we generalize and implement without directly naming inches, feet, pounds, or ounces, or if we implement specifically:

  • Feet and inches:
    image
    image
  • Pounds and ounces:
    image
    image

A potential related issue is sub-sub-units. Although the only case I can think of is degrees-minutes-seconds, and these are not supported units from the bot, the bot would not support converting 19 degrees, 34 minutes, and 12 seconds into 0.342 radians. However, as no units that are (to my knowledge) regularly used as sub-sub-units are implemented in the bot, I do not propose solving this issue at the present. Instead, I mention it only because if it were to be supported, its implementation would, in all likelihood, directly involve the code used to solve this issue.

Proposed Fixes

  • Create a regex that searches for something imperial followed by an optional and, any whitespace, and another something imperial and processes these before the individual units are processed. If the units are of a different type from each other (e.g, I am 6 feet and 220 pounds), the additional check must fallback to the main check.
  • Invoke the above additional regex check into the main regex checker by enclosing the segment after the first something imperial in an optional (occurs 0 or 1 times) flag.
  • Invoke the above additional regex check into the main regex checker by enclosing the segment after the first something imperial in a non-negative loop (occurs 0 or more times) flag.
  • Record the replacements of individual imperial numbers that have been made, and then if more than one of the same type has been made, check that they are consecutive and separated by a trivial separator string (just whitespace and and). When these checks are satisfied, replace the replacements with the sum of their values.

Cheers!

@Randall-Scharpf Randall-Scharpf linked a pull request Aug 9, 2021 that will close this issue
@jamesgeddes
Copy link

jamesgeddes commented Sep 28, 2021

I would also like to add that stone and pounds suffers from the same issue.

User:

12st 11.2lbs.

UnitCorrector:

I think user meant to say:

76.2kg 5.08kg

In Britain, Imperial weight is usually given in stone and pounds, rather than pounds alone. Because Britain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants