-
Notifications
You must be signed in to change notification settings - Fork 729
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
preserve language tags #1354
preserve language tags #1354
Conversation
|
3511882
to
046998a
Compare
This is unfortunately not correct when the language tag contains a private use subtag ( |
src/impl/locale.js
Outdated
@@ -75,22 +75,26 @@ function parseLocaleString(localeStr) { | |||
return [localeStr]; | |||
} else { | |||
let options; | |||
let selectedStr; | |||
const smaller = localeStr.substring(0, uIndex); |
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.
You can now move this substring into the catch
. It was only outside of it because we used it in the return
.
@@ -695,7 +695,7 @@ test("DateTime.fromObject overrides the locale string with explicit settings", ( | |||
} | |||
); | |||
|
|||
expect(res.locale).toBe("be"); | |||
expect(res.locale).toBe("be-u-ca-coptic-nu-mong"); | |||
expect(res.outputCalendar).toBe("islamic"); | |||
expect(res.numberingSystem).toBe("thai"); | |||
}); |
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.
Can you also add a test here where you set the locale
to something with a calendar and an explicit outputCalendar
and show that it doesn't blow up (I don't care which calendar "wins", but the test should document the result, i.e. which calendar comes out of the resolved options)
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 this is that test?
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.
oh, ha, you're right. My bad
@diesieben07 do you have any suggestion for what should be done? Does this need to support private use subtags? |
You could either detect the
Probably not. But currently this code is a regression, because previously they were just ignored and now they cause the Edit: I just want to clarify, that I am not a maintainer for this project, so I do not have any decision power here! |
Looks good. Thanks! |
Fixes: #1335