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

Fix incorrect expectation of resolvedOptions.locale while no "hour" in options #2035

Closed
wants to merge 1 commit into from

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Jan 17, 2019

Notice the comment stated:
// Also test the case when no hour option is present at all.
// The resolved options don't include hour12 and hourCycle if the date-time
// formatter doesn't include an hour option. This restriction doesn't apply
// to the hc Unicode extension value.

If so, for that test without hour: in options, the resolved locale should equal to locale, not expectedLocale
@littledan @anba @Ms2ger

Notice the comment stated:
  // Also test the case when no hour option is present at all.
  // The resolved options don't include hour12 and hourCycle if the date-time
  // formatter doesn't include an hour option. This restriction doesn't apply
  // to the hc Unicode extension value.

If so, for that test without hour:, it should equal to locale, not expectedLocale
@FrankYFTang FrankYFTang changed the title Fix incorrect expectation Fix incorrect expectation of resolvedOptions.locale while no "hour" in options Jan 17, 2019
@FrankYFTang
Copy link
Contributor Author

@gsathya @jungshik

@FrankYFTang
Copy link
Contributor Author

@zbraniecki

@anba
Copy link
Contributor

anba commented Jan 21, 2019

No, the test is correct as is.

The comment tries to explain that even though the resolved options don't contain hour12 and hourCycle when hour is not present, the resolved locale still contains the hc Unicode extension key.

InitializeDateTimeFormat, step 29 only sets [[HourCycle]] when [[Hour]] is not undefined, otherwise (step 30) [[HourCycle]] is set to undefined, too. That means for Intl.DateTimeFormat.prototype.resolvedOptions, the resolved options only contain hour12 and hourCycle when [[Hour]] is not undefined.

The resolved locale however is computed before we know whether or not [[Hour]] is present in the date-time formatter, cf. InitializeDateTimeFormat, step 11. So the hc Unicode extension key will be present in the resolved locale even when [[Hour]] is not present.

@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Jan 21, 2019

OK, following your logic- could you show me what should the following 8 cases output?

A (new Intl.DateTimeFormat("en-u-hc-h11", { hour12: false, hourCycle: "h23"}).resolvedOptions()).locale
B (new Intl.DateTimeFormat("en-u-hc-h11", { hour12: false}).resolvedOptions()).locale
C (new Intl.DateTimeFormat("en-u-hc-h11", { hourCycle: "h23"}).resolvedOptions()).locale
D (new Intl.DateTimeFormat("en-u-hc-h11").resolvedOptions()).locale
E (new Intl.DateTimeFormat("en", { hour12: false, hourCycle: "h23"}).resolvedOptions()).locale
F (new Intl.DateTimeFormat("en", { hour12: false}).resolvedOptions()).locale
G (new Intl.DateTimeFormat("en", { hourCycle: "h23"}).resolvedOptions()).locale
H (new Intl.DateTimeFormat("en").resolvedOptions()).locale

@FrankYFTang
Copy link
Contributor Author

OK, following your logic- could you show me what should the following 8 cases output?

A (new Intl.DateTimeFormat("en-u-hc-h11", { hour12: false, hourCycle: "h23"}).resolvedOptions()).locale
B (new Intl.DateTimeFormat("en-u-hc-h11", { hour12: false}).resolvedOptions()).locale
C (new Intl.DateTimeFormat("en-u-hc-h11", { hourCycle: "h23"}).resolvedOptions()).locale
D (new Intl.DateTimeFormat("en-u-hc-h11").resolvedOptions()).locale
E (new Intl.DateTimeFormat("en", { hour12: false, hourCycle: "h23"}).resolvedOptions()).locale
F (new Intl.DateTimeFormat("en", { hour12: false}).resolvedOptions()).locale
G (new Intl.DateTimeFormat("en", { hourCycle: "h23"}).resolvedOptions()).locale
H (new Intl.DateTimeFormat("en").resolvedOptions()).locale

@anba Could you just show me what do you think the above 8 cases should output, one by one?

@FrankYFTang
Copy link
Contributor Author

@anba

So the hc Unicode extension key will be present in the resolved locale even when [[Hour]] is not present.

I agree with your statement quoted above, and that is exactly why the tests was wrong and need this PR. The test is currently assert the resolvedOptoins.locale is the same as "expectedLocale" (the 2nd) argument, which if you read the caller's code, are "defaultLocale" for all except one case. And defaultLocale is the one WITHOUT hc Unicode extension key, NOT the one WITH the hc Unicode extension key.

@anba
Copy link
Contributor

anba commented Jan 24, 2019

A (new Intl.DateTimeFormat("en-u-hc-h11", { hour12: false, hourCycle: "h23"}).resolvedOptions()).locale

"en".

  • 12.1.1, steps 8-9 set opt.[[hc]] to null
  • null is in [[LocaleData]][locale].hc (12.3.3)
  • 9.2.6, step 8.h applies, because r.[[hc]] == "h11", 8.h.ii.1.a sets value = "hc11" and supportedExtension = "-hc-h11".
  • 9.2.6, step 8.i also applies, SameValue in step 8.i.iii.1 returns false, therefore value = null and supportedExtension = "".
  • 9.2.6., step 8.k now has supportedExtension = "".

B (new Intl.DateTimeFormat("en-u-hc-h11", { hour12: false}).resolvedOptions()).locale

"en".

  • 12.1.1, steps 8-9 set opt.[[hc]] to null
  • null is in [[LocaleData]][locale].hc (12.3.3)
  • 9.2.6, step 8.h applies, because r.[[hc]] == "h11", 8.h.ii.1.a sets value = "hc11" and supportedExtension = "-hc-h11".
  • 9.2.6, step 8.i also applies, SameValue in step 8.i.iii.1 returns false, therefore value = null and supportedExtension = "".
  • 9.2.6., step 8.k now has supportedExtension = "".

C (new Intl.DateTimeFormat("en-u-hc-h11", { hourCycle: "h23"}).resolvedOptions()).locale

"en".

  • 12.1.1, step 9 sets opt.[[hc]] to "h23"
  • "h23" is in [[LocaleData]][locale].hc (12.3.3)
  • 9.2.6, step 8.h applies, because r.[[hc]] == "h11", 8.h.ii.1.a sets value = "hc11" and supportedExtension = "-hc-h11"
  • 9.2.6, step 8.i also applies, SameValue in step 8.i.iii.1 returns false, therefore value = "h23" and supportedExtension = "".
  • 9.2.6., step 8.k now has supportedExtension = "".

D (new Intl.DateTimeFormat("en-u-hc-h11").resolvedOptions()).locale

"en-u-hc-h11".

  • 12.1.1, step 9 sets opt.[[hc]] to undefined
  • undefined is not in [[LocaleData]][locale].hc (12.3.3)
  • 9.2.6, step 8.h applies, because r.[[hc]] == "h11", 8.h.ii.1.a sets value = "hc11" and supportedExtension = "-hc-h11"
  • 9.2.6, step 8.i.iii doesn't apply because optionsValue == undefined is not in keyLocaleData
  • 9.2.6, step 8.k has supportedExtension == "hc11".

E (new Intl.DateTimeFormat("en", { hour12: false, hourCycle: "h23"}).resolvedOptions()).locale

"en".

  • 12.1.1, steps 8-9 set opt.[[hc]] to null
  • null is in [[LocaleData]][locale].hc (12.3.3)
  • 9.2.6, step 8.h doesn't apply because r doesn't have a [[hc]] field
  • 9.2.6, step 8.i applies, SameValue in step 8.i.iii.1 returns false, so supportedExtension is still the empty string
  • 9.2.6, step 8.k has supportedExtension == "".

F (new Intl.DateTimeFormat("en", { hour12: false}).resolvedOptions()).locale

Same as E.

G (new Intl.DateTimeFormat("en", { hourCycle: "h23"}).resolvedOptions()).locale

"en".

  • 12.1.1, step 9 sets opt.[[hc]] to "h23"
  • "h23" is in [[LocaleData]][locale].hc (12.3.3)
  • 9.2.6, step 8.h doesn't apply because r doesn't have a [[hc]] field
  • 9.2.6, step 8.i applies, SameValue in step 8.i.iii.1 returns false, therefore value = "h23" and supportedExtension = ""
  • 9.2.6, step 8.k has supportedExtension == "".

H (new Intl.DateTimeFormat("en").resolvedOptions()).locale

"en"

  • 12.1.1, step 9 sets opt.[[hc]] to undefined
  • undefined is not in [[LocaleData]][locale].hc (12.3.3)
  • 9.2.6, step 8.h doesn't apply because r doesn't have a [[hc]] field.
  • 9.2.6, step 8.i.iii doesn't apply because optionsValue == undefined is not in keyLocaleData
  • 9.2.6, step 8.k has supportedExtension == "".

@anba
Copy link
Contributor

anba commented Jan 24, 2019

So the test is still correct.

@anba
Copy link
Contributor

anba commented Jan 24, 2019

Basically an hour12 option always overrides an hourCycle option. Additionally hour12 and hourCycle both clear out any existing Unicode extension key in the input locale.

@FrankYFTang
Copy link
Contributor Author

@anba thank you so much. This part of the spec is very tricky to figure. I think the comment below really confuse me

// Also test the case when no hour option is present at all.
// The resolved options don't include hour12 and hourCycle if the date-time
// formatter doesn't include an hour option. This restriction doesn't apply
// to the hc Unicode extension value.

Maybe we need to change the comment to make it clear? But I now agree w/ you this test is according to the spec. Sorry for bothering yo about this.

@FrankYFTang FrankYFTang deleted the patch-1 branch January 24, 2019 22:02
@anba
Copy link
Contributor

anba commented Jan 31, 2019

Sorry for bothering yo about this.

No worries. The ResolveLocale operation is definitely a bit tricky, I think I only started to understand it completely after I had implemented it from scratch. 😄

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.

None yet

2 participants