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

Implements the ECMA402 trait for Intl.DateTimeFormat. #165

Merged
merged 2 commits into from
Oct 22, 2020

Conversation

filmil
Copy link
Member

@filmil filmil commented Aug 28, 2020

Closes: #164

@google-cla google-cla bot added the cla: yes CLA signed label Aug 28, 2020
@filmil filmil self-assigned this Aug 28, 2020
@filmil filmil requested a review from kpozin August 28, 2020 19:27
@filmil
Copy link
Member Author

filmil commented Aug 28, 2020

@zbraniecki PTAL

I am still not sure about the options -- perhaps a better choice can be made.

ecma402_traits/src/datetimeformat.rs Outdated Show resolved Hide resolved
ecma402_traits/src/datetimeformat.rs Show resolved Hide resolved
ecma402_traits/src/datetimeformat.rs Outdated Show resolved Hide resolved
ecma402_traits/src/datetimeformat.rs Show resolved Hide resolved
ecma402_traits/src/datetimeformat.rs Outdated Show resolved Hide resolved
ecma402_traits/src/datetimeformat.rs Show resolved Hide resolved
@filmil
Copy link
Member Author

filmil commented Sep 11, 2020

Would it be something worth considering to align here (or there)?

tl;dr: I'd attempt to do (1) from below, unless you think a better approach is available.

Ideally, an interface definition should dictate what the implementation has to conform to. So if at all possible, I'd prefer to try to modify the implementation in icu4x to conform.

In practice, of course, we extract or retrofit interfaces.

I hope that it would be possible to:

  1. Try to have the trait definition dictate the implementation if possible. I did that in rust_icu, sometimes at the expense of having to write glue code.
  2. Try to keep the trait definition clean of implementation as much as possible.

Now, I think we can't go around implementing the options enum, since the configuration is part of the API surface, unless we're willing to have the trait be "stringly" typed. But let me know if I'm mistaken.

It seemed like a reasonable tradeoff to allow for impls of Options enum to continue existing here, as they are part of the API surface and all spec-conforming implementations must have them anyways.

@zbraniecki
Copy link

I'm mostly talking about "nesting" of options into Style vs ComponentsBag. Do you think it may be a good option here?

@filmil filmil force-pushed the datetimeformat-ecma402-trait branch from 923cda7 to f2fe47b Compare September 29, 2020 22:26
@filmil filmil merged commit 793ba74 into google:master Oct 22, 2020
@filmil filmil deleted the datetimeformat-ecma402-trait branch September 17, 2021 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the ECMA402 Intl.DateTimeFormat API
3 participants