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

Add likely_subtags: data generation, node executor #93

Merged
merged 9 commits into from
Aug 23, 2023

Conversation

sven-oly
Copy link
Collaborator

No description provided.

@sven-oly
Copy link
Collaborator Author

This uses the latest CLDR data in https://github.com/unicode-org/cldr/blob/main/common/testData/localeIdentifiers/likelySubtags.txt for checking Node's minimize and maximize locale tags.

Comment on lines 210 to 211
# Create 3 minimize tests
expected = tags[2]
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: you should read tags[3] and put it in an option called minimizeFavorScript

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Will do!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Comment on lines 219 to 221
verify = {'label': label,
'verify': expected
}
Copy link
Member

Choose a reason for hiding this comment

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

Please note the highlighted portions of the notice at the top of the test data file:

Source ; AddLikely ; RemoveFavorScript ; RemoveFavorRegion
Source: a locale to which the following operations are applied.
AddLikely: the result of the Add Likely Subtags.
If Add Likely Subtags fails, then “FAIL”.
RemoveFavorScript: Remove Likely Subtags, when the script is favored.
Only included when different than AddLikely.
RemoveFavorRegion: Remove Likely Subtags, when the region is favored.
Only included when different than RemoveFavorScript.

I don't immediately see where you are doing the "only included when..." portion of the instructions. Most but not all cases have data populated for RemoveFavorScript, and only a few have data populated for RemoveFavorRegion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated tests and code in latest commits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to handle non-included values, falling back to previous.

@sven-oly sven-oly mentioned this pull request Aug 19, 2023
if line.find('FAIL') >= 0:
# What do do with this?
pass

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to ignore this comment.


Right now you have:

{ label: "123", verify: "en-Latn-US" }

The expected could be like:

{ label: "123", verify: { locale: "en-Latn-US" } }

Instead it could be:

{ label: "123", verify: { error: true } }

Or, you just output the string "FAIL" from the executor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this, using "FAIL" as if it's expected data.

test_option === 'minimize') {
result_locale = intl_locale.minimize().baseName;
} else if (test_option === 'minimizeFavorRegion') {
result_locale = intl_locale.minimizeFavorRegion().baseName;
Copy link
Member

Choose a reason for hiding this comment

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

Note: there is no Intl.Locale.prototype.minizeFavorRegion; this should be unsupported

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated now.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see where this is fixed but OK to fix later

# Create minimize tests - default is RemoveFavorScript
if tags[2]:
remove_favor_script = tags[2]
for tag in tags:
Copy link
Member

Choose a reason for hiding this comment

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

This generates extra cases for each line. Probably not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simplified, creating maximum of 3 cases per source locale.

pass

# Create minimize tests - default is RemoveFavorScript
if tags[2]:
Copy link
Member

Choose a reason for hiding this comment

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

tags[2] should inherit from tags[1] and tags[3] should inherit from tags[2].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@sven-oly
Copy link
Collaborator Author

Much better now! PTAL

test_option === 'minimize') {
result_locale = intl_locale.minimize().baseName;
} else if (test_option === 'minimizeFavorRegion') {
result_locale = intl_locale.minimizeFavorRegion().baseName;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where this is fixed but OK to fix later

@sven-oly sven-oly merged commit 8af1d19 into unicode-org:main Aug 23, 2023
4 checks passed
@sven-oly sven-oly deleted the likelySubtags branch August 23, 2023 22:06
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.

3 participants