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

Provide a getter for the resolved options for the collator #4047

Merged
merged 17 commits into from
Jan 16, 2024

Conversation

hsivonen
Copy link
Member

Fixes #3880

(Before merging, we should decide if we want the return type to be CollatorOptions of if we want a ResolvedCollatorOptions that doesn't wrap each field in Option.)

@hsivonen hsivonen marked this pull request as draft September 15, 2023 12:01
@hsivonen
Copy link
Member Author

It looks like CI wants the Diplomat wrapper to go into the same PR.

I'm converting this to draft until it's been decided whether this should return CollatorOptions or a new ResolvedCollatorOptions that doesn't wrap each field in Option.

@robertbastian robertbastian added discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting labels Oct 5, 2023
@sffc
Copy link
Member

sffc commented Oct 5, 2023

Discuss with:

@zbraniecki
Copy link
Member

In my prototype of icu_preferences I moved in the direction of a macro generating resolved options without Option. I think this is the direction we should take here.

@hsivonen are you opinionated in the other direction?

@hsivonen
Copy link
Member Author

hsivonen commented Oct 9, 2023

In my prototype of icu_preferences I moved in the direction of a macro generating resolved options without Option. I think this is the direction we should take here.

@hsivonen are you opinionated in the other direction?

I am not. That is, without Option works for me. Thanks.

@sffc sffc added the discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band label Nov 2, 2023
@sffc
Copy link
Member

sffc commented Nov 7, 2023

I'll weigh in here. I think the input and output options should be separate structs.

In ECMA-402, the input and output structs are sometimes not the same. For example, Intl.NumberFormat takes localeMatcher as an input option but locale as a resolved option (though we have other mechanisms in icu4x for dealing with locale matching and supported locales).

But more importantly, the semantics of resolved options are different. The required versus optional fields are not the same. For example, in Intl.NumberFormat, currency is optional in both input and output, but style is always present in output. (This raises an interesting question about whether we should express the dependence between options in the type system so that currency is required when style = "currency"; I think not, just return an error according to the ECMAScript specification.)

This would imply that the options should also be different structs across FFI.

Ideally these FFI structs should also include optional fields, but that is blocked on rust-diplomat/diplomat#132.

@hsivonen
Copy link
Member Author

hsivonen commented Nov 8, 2023

This would imply that the options should also be different structs across FFI.

I have now made the FFI types distinct from each other even though the types are, for now, identical except for the name.

Ideally these FFI structs should also include optional fields, but that is blocked on rust-diplomat/diplomat#132.

I have not duplicated the enums that go in the FFI option bags, so on the resolved side Auto is guaranteed-unused, but that's not communicated via the type system.

@sffc sffc self-requested a review November 8, 2023 15:12
@sffc sffc removed discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band labels Nov 9, 2023
@sffc
Copy link
Member

sffc commented Nov 9, 2023

As discussed on 2023-11-09, we're happy with the direction of this PR.

@hsivonen
Copy link
Member Author

Blocked on #4326. It appears that dart provided by the flutter snap doesn't satisfy the formatting expectations of CI.

@hsivonen hsivonen marked this pull request as ready for review November 28, 2023 12:18
@hsivonen
Copy link
Member Author

Looks like merge conflicts show up faster than my cycle to check back here for the CI results. I'll merge main again.

@Manishearth Manishearth removed their request for review November 28, 2023 16:12
sffc
sffc previously approved these changes Jan 5, 2024
@hsivonen
Copy link
Member Author

If/when this get re-approved, please push the landing button, too, so that this doesn't get out of sync with main before I have an opportunity to push the landing button.

@Manishearth Manishearth merged commit e2f5405 into unicode-org:main Jan 16, 2024
29 checks passed
@hsivonen hsivonen deleted the collatorresolvedoptions branch January 16, 2024 09:30
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.

Collator needs a way to extract resolved options for ECMA-402
5 participants