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

CLDR-17298 Fix problem with unit exponents #4071

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

macchiati
Copy link
Member

This is a fix for the problem noted in https://unicode-org.atlassian.net/browse/CLDR-17298?focusedCommentId=177586

CLDR-17298

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

btangmu
btangmu previously approved these changes Sep 24, 2024
@macchiati
Copy link
Member Author

Hi, I did some wording cleanup; could someone re-review?

@@ -951,15 +951,17 @@ Some of the constraints reference data from the unitIdComponents in [Unit_Conver
</ul></td></tr>

<tr><td><a name='unit_constant' href='unit_constant'>unit_constant</a></td><td>:=</td>
<td>("1"[0-9]+ | [2-9][0-9]*)("e" ("1"[0-9]+ | [2-9][0-9]*))?
<td>[1-9][0-9]* ("e" [1-9][0-9]*)?
Copy link
Contributor

Choose a reason for hiding this comment

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

this will allow "1" in "per-1" . Is that ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is disallowed by the constraint below.

<li><em>Note:</em> The <code>e</code> notation is optional: per-100-kilometer and per-1e2-kilometer are equivalent unit_identifiers.</li>
<li><em>Note:</em> When constructing identifiers, exponents should be greater than 3 and multiples of 3, even though parsers must accept the wider range.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

but in the previous line you have the example of "per-1e2-kilometer" 1e2 is not "exponents ... greater than 3 and multiples of 3", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. They are equivalent. The following line makes it clear that the 100 is the preferred form in that case for generators of unit identifiers, even thought 1e2 must be accepted by parsers of unit identifiers.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think what you're saying in this comment is clearer than what you have in the spec right now.

  2. Is anything in the tooling enforcing these constraints, or is something else (e.g., the review process) supposed to do that?

Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

LGTM except for a couple comments...

<li><em>Note:</em> The <code>e</code> notation is optional: per-100-kilometer and per-1e2-kilometer are equivalent unit_identifiers.</li>
<li><em>Note:</em> When constructing identifiers, exponents should be greater than 3 and multiples of 3, even though parsers must accept the wider range.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think what you're saying in this comment is clearer than what you have in the spec right now.

  2. Is anything in the tooling enforcing these constraints, or is something else (e.g., the review process) supposed to do that?

@macchiati macchiati merged commit 54ee48b into main Sep 25, 2024
13 checks passed
@macchiati
Copy link
Member Author

Merging now, but can clean up wording later

@macchiati macchiati deleted the CLDR-17298-macchiati-Fix-unit-exponents branch September 25, 2024 14:00
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.

5 participants