-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Node errors for locale display names to 'unsupported' #319
base: main
Are you sure you want to change the base?
Conversation
Fixes #318 |
executors/node/localedisplaynames.js
Outdated
if (error instanceof RangeError) { | ||
// The locale can't be handled for some reason! | ||
outputLine["error_type"] = 'unsupported'; | ||
outputLine["unsupported"] = error.toString(); | ||
outputLine["error_detail"] = 'unsupported locale'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error instanceof RangeError
seems a bit broad. I suggest a narrower case for returning unsupported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, there are two types of failures right now. Here's what I see being caught as exceptions:
- Unexpected options in the locale spec, e.g., "label":"0009","language_label":"en-u-nu-deva-t-de","locale_label":"en"
--> "RangeError: invalid_argument"
- "root" locale --> "RangeError: invalid_argument" in the constructor.
Should either of these be "unsupported"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think classifying these as "unsupported" is correct. This will resolve a large number of "errors" --> "unsupported", i.e.,
Report created: 2024-10-07 10:36
1,990 attempted. Pass: 1,460, Fail: 214, Errors: 316, Unsupported: 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL!
Please review again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ready. The problem is that the locale is "root" which is not a valid locale.
Root locale was removed in PR #341 |
This fixes all the "error" cases in NodeJs for locale display names, moving them to "unsupported". This works for unsupported locale options and also the "root" locale.