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

Remove pattern string constructor from rust_icu_udat #2

Open
sffc opened this issue Nov 14, 2019 · 4 comments
Open

Remove pattern string constructor from rust_icu_udat #2

sffc opened this issue Nov 14, 2019 · 4 comments
Labels
discuss before start The issues isn't quite ready to be fixed, there are open questions to be discussed enhancement New feature or request help wanted Extra attention is needed

Comments

@sffc
Copy link

sffc commented Nov 14, 2019

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
Copy link
Member

filmil commented Nov 14, 2019

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
Copy link
Author

sffc commented Nov 14, 2019

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

@filmil
Copy link
Member

filmil commented Nov 14, 2019

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:

  • making the ICU functionality available to rust; and
  • ECMA-402 conformance

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
Copy link
Author

sffc commented Nov 16, 2019

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

@sffc sffc changed the title Feedback on rust_icu_udat Remove pattern string constructor from rust_icu_udat Nov 16, 2019
@filmil filmil added the enhancement New feature or request label May 8, 2020
@filmil filmil added the help wanted Extra attention is needed label Aug 27, 2020
@filmil filmil added the discuss before start The issues isn't quite ready to be fixed, there are open questions to be discussed label Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss before start The issues isn't quite ready to be fixed, there are open questions to be discussed enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants