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

preserve language tags #1354

Merged
merged 3 commits into from Jan 3, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Expand Up @@ -15,7 +15,7 @@ Here are some vague notes on Luxon's design philosophy:

## Building and testing

Building and testing is done through npm scripts. The tests run in Node and require Node 10+ with full-icu support. This is because some of the features available in Luxon (like internationalization and time zones) need that stuff and we test it all. On any platform, if you have Node 10 installed with full-icu, you're good to go; just run npm scripts like `npm run test`. But you probably don't have that, so read on.
Building and testing is done through npm scripts. The tests run in Node and require Node 18 with full-icu support. This is because some of the features available in Luxon (like internationalization and time zones) need that stuff and we test it all. On any platform, if you have Node 18 installed with full-icu, you're good to go; just run `scripts/test`. But you probably don't have that, so read on.

### OSX

Expand Down
10 changes: 7 additions & 3 deletions src/impl/locale.js
Expand Up @@ -75,22 +75,26 @@ function parseLocaleString(localeStr) {
return [localeStr];
} else {
let options;
let selectedStr;
const smaller = localeStr.substring(0, uIndex);
Copy link
Member

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.

try {
options = getCachedDTF(localeStr).resolvedOptions();
selectedStr = localeStr;
} catch (e) {
options = getCachedDTF(smaller).resolvedOptions();
selectedStr = smaller;
}

const { numberingSystem, calendar } = options;
// return the smaller one so that we can append the calendar and numbering overrides to it
return [smaller, numberingSystem, calendar];
return [selectedStr, numberingSystem, calendar];
}
}

function intlConfigString(localeStr, numberingSystem, outputCalendar) {
if (outputCalendar || numberingSystem) {
localeStr += "-u";
if (!localeStr.includes("-u-")) {
localeStr += "-u";
}

if (outputCalendar) {
localeStr += `-ca-${outputCalendar}`;
Expand Down
6 changes: 3 additions & 3 deletions test/datetime/create.test.js
Expand Up @@ -663,7 +663,7 @@ test("DateTime.fromObject accepts a locale", () => {

test("DateTime.fromObject accepts a locale with calendar and numbering identifiers", () => {
const res = DateTime.fromObject({}, { locale: "be-u-ca-coptic-nu-mong" });
expect(res.locale).toBe("be");
expect(res.locale).toBe("be-u-ca-coptic-nu-mong");
expect(res.outputCalendar).toBe("coptic");
expect(res.numberingSystem).toBe("mong");
});
Expand All @@ -677,7 +677,7 @@ test("DateTime.fromObject accepts a locale string with weird junk in it", () =>
}
);

expect(res.locale).toBe("be");
expect(res.locale).toBe("be-u-ca-coptic-ca-islamic");

// "coptic" is right, but some versions of Node 10 give "gregory"
expect(res.outputCalendar === "gregory" || res.outputCalendar === "coptic").toBe(true);
Expand All @@ -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");
});
Copy link
Member

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)

Copy link
Contributor Author

@devsnek devsnek Dec 31, 2022

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?

Copy link
Member

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

Expand Down
6 changes: 6 additions & 0 deletions test/datetime/format.test.js
Expand Up @@ -415,6 +415,12 @@ test("DateTime#toLocaleString uses locale-appropriate time formats", () => {
expect(dt.reconfigure({ locale: "es" }).toLocaleString(DateTime.TIME_24_SIMPLE)).toBe("9:23");
});

test("DateTime#toLocaleString() respects language tags", () => {
expect(dt.reconfigure({ locale: "en-US-u-hc-h23" }).toLocaleString(DateTime.TIME_SIMPLE)).toBe(
"09:23"
);
});

test("DateTime#toLocaleString() accepts a zone even when the zone is set", () => {
expect(
dt.toLocaleString({
Expand Down