search

google / rust_icu

rust_icu: rust bindings for ICU (International Components for Unicode) library
Apache License 2.0
113 stars 28 forks source link

Remove pattern string constructor from rust_icu_udat #2

Open sffc opened 4 years ago

sffc commented 4 years ago

ECMA-402 does not and probably will not add support for parsing. The bottom line is that parsing of localized strings is a hard problem that is best solved by simply building your application to avoid having to do it. Do you really need to support parsing in the wrapper?

ECMA-402 also does and probably will not support patterns. ICU has long recommended that people don't use patterns; they should use skeletons instead. Do you really need to support patterns?

Speaking of skeletons, you don't have an API for skeletons right now. Could you remove your pattern API and replace it with a skeleton API?

filmil commented 4 years ago

Fuchsia currently uses the patterns. I'm open to reshaping the API.

Also, I don't think that parsing is used except in tests; the idea was to be able to go from the serialization to the struct and back. Feel free to file a separate issue for skeleton support.

sffc commented 4 years ago

Why does Fuchsia use patterns? This would be a good opportunity to migrate them off of patterns.

filmil commented 4 years ago

Why does Fuchsia use patterns? This would be a good opportunity to migrate them off of patterns.

By accident. Here is the code. I rewrote the C++ version of the "wisdom" server in rust as a proof of concept for ICU support in rust. This was the shortest way to rewrite, and at the time I was not looking explicitly into what the best API would be.

Also note that there are two goals, which at least in my mind are equally interesting:

In this particular case I think we can go for the latter and drop the former. If you have suggestions, design proposals and pull requests are most welcome. I think we can keep this issue open until this is implemented.

sffc commented 4 years ago

Thanks for the call site! It looks like they aren't actually using patterns; they're using dateStyle/timeStyle. Making an API that lets you specify dateStyle/timeStyle (mapping that to the pattern constructor under the hood) would be 402-compliant.

You already have a dateStyle/timeStyle API:

https://github.com/google/rust_icu/blob/master/rust_icu_udat/src/lib.rs#L72

Why do you need the one a few lines down that accepts a pattern string? That's the one that is problematic, because there's no good way for a user to construct such a pattern string in a locale-agnostic way.

https://github.com/google/rust_icu/blob/master/rust_icu_udat/src/lib.rs#L99