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

Ecma402 traits #125

Merged
merged 2 commits into from Jun 13, 2020
Merged

Ecma402 traits #125

merged 2 commits into from Jun 13, 2020

Conversation

filmil
Copy link
Member

@filmil filmil commented Jun 12, 2020

Here is a draft implementation of ECMA 402 API surface that was proposed here: https://github.com/google/rust_icu/blob/master/experimental/ecma402_listformat/src/lib.rs

Before I equip it with better docs and README content, initial thoughts from y'all would be welcome.

@kpozin uloc now depends on the ecma402_traits, which means we need to import it in Fuchsia too. It seemed that if I created a new aliased type for the locale, we'd not get much in return except we'd get the decoupling.

@filmil filmil force-pushed the ecma402-traits branch 5 times, most recently from 764bcb4 to 0cf05ec Compare June 12, 2020 01:28
@filmil filmil requested a review from kpozin June 12, 2020 18:57
@filmil
Copy link
Member Author

filmil commented Jun 12, 2020

@zbraniecki if you take a look, feel free to ignore the rust_icu specific changes and focus on the implementation only. The goal here was not to have a super-tight implementation, but one that demonstrates how the API surface works.

@filmil
Copy link
Member Author

filmil commented Jun 12, 2020

An amusing bit is that the implementation revealed a few practical issues that rust_icu must resolve in order to support ECMA 402. One of them is that the list formatting support we need for ECMA 402 is available only since ICU 67 (!), and that in practice, systems use ICUs as far back as ICU 63. (each version increment is about 6 months IIRC).

So, a few conditional compilation experiments later, there's something that works on ICU 67, and gives suboptimal but still useful results on ICU 66 and lower.

rust_icu_ecma402/src/lib.rs Outdated Show resolved Hide resolved
rust_icu_ecma402/src/listformat.rs Show resolved Hide resolved
use ecma402_traits::listformat::options;

// Converts the param types from ECMA-402 into ICU.
pub fn to_icu_width(style: &options::Style) -> usys::UListFormatterWidth {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm conflicted. If we're making rust_icu depend on ecma402_traits anyway, maybe we should add standard From/To conversion traits to rust_icu. (This would require some hacks for bindgen code; cf. rust-lang/rust-bindgen#1089.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This would be in a later CL, of course.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how automated bindgen would help in this case though, care to elaborate?

But I share your concern -- it reads like an inverted dependency on (fairly low level) rust_icu that it should conform to a set of high level conceptual APIs (ecma 402).

On the other hand, reimplementing Locale in a rust_icu backed implementation of ecma402 seems like we're, well, redoing the work that already exists in rust_icu.

@filmil
Copy link
Member Author

filmil commented Jun 12, 2020 via email

Copy link

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

ecma402_traits/src/listformat.rs Show resolved Hide resolved
rust_icu_ecma402/src/listformat.rs Show resolved Hide resolved
rust_icu_ecma402/src/listformat.rs Show resolved Hide resolved
rust_icu_ecma402/src/listformat.rs Show resolved Hide resolved
The motivation is that the proposals are not experimental *code*, but
rather, they are documents that are meant to be read, and happen to have
some bits that you can copy over to the rust playground.

So in that respect, the texts are no different than any other doc you
may write, hence the move to proposals.
filmil added a commit to filmil/rust_icu that referenced this pull request Jun 13, 2020
This change contributes an implementation of the [Intl.ListFormat
API][lfapi], as prescribed by [ECMA-402][ecma].

The ECMA-402 API surface is developed in lock-step with a `rust_icu`
backed implementation.

The approach is documented [here][appr].

  [lfapi]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat
  [ecma]: https://www.ecma-international.org/publications/standards/Ecma-402.htm
  [appr]: https://raw.githubusercontent.com/google/rust_icu/master/experimental/ecma402_listformat/src/lib.rs

See issue google#125
This change contributes an implementation of the [Intl.ListFormat
API][lfapi], as prescribed by [ECMA-402][ecma].

The ECMA-402 API surface is developed in lock-step with a `rust_icu`
backed implementation.

The approach is documented [here][appr].

  [lfapi]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat
  [ecma]: https://www.ecma-international.org/publications/standards/Ecma-402.htm
  [appr]: https://raw.githubusercontent.com/google/rust_icu/master/experimental/ecma402_listformat/src/lib.rs

See issue google#125
@filmil filmil merged commit a77bd39 into google:master Jun 13, 2020
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

3 participants