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

Add example for icu_locale #250

Merged
merged 4 commits into from
Sep 22, 2020
Merged

Conversation

zbraniecki
Copy link
Member

#187 for Locale.

@zbraniecki
Copy link
Member Author

Without icu_locale: 321464 bytes
With: 327856 bytes

The crate adds 6392 bytes.

@coveralls
Copy link

coveralls commented Sep 17, 2020

Pull Request Test Coverage Report for Build e169eecbd546ca58e8630512584881326239a090-PR-250

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 78.936%

Totals Coverage Status
Change from base Build d6734764840095914f2c9be32a7f27eb544688cd: -0.02%
Covered Lines: 2151
Relevant Lines: 2725

💛 - Coveralls

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

This example seems a little synthetic. Can you make it "do" something? For example, given a list of locale strings, find all the ones with language subtag "en", and then canonicalize that filtered list. You end up with a program that calculates all forms of English your app supports.


{
// 2. Filter list of identifiers that match a given Language Identifier.
let reference: LanguageIdentifier = "en-US"
Copy link
Member

Choose a reason for hiding this comment

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

Use your macro here instead? There should no longer be any reason to hard-code a "...".parse().expect("...") pattern for LanguageIdentifier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Once we merge the macros! :)

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to use the macros in this example. Hopefully the macros will be checked in soon.


{
// 3. Filter list of identifiers that match a given language subtag.
let reference: subtags::Language = "en".parse().expect("Failed to parse Language");
Copy link
Member

Choose a reason for hiding this comment

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

Likewise: use language! (or icu_language!)

@zbraniecki
Copy link
Member Author

This example seems a little synthetic. Can you make it "do" something? For example, given a list of locale strings, find all the ones with language subtag "en", and then canonicalize that filtered list. You end up with a program that calculates all forms of English your app supports.

Good idea! I'll improve the example tmrw.

@zbraniecki zbraniecki requested a review from sffc September 19, 2020 18:53
@zbraniecki
Copy link
Member Author

Refactored the example according to @sffc's suggestion.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/locale/examples/filter_langids.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/locale/examples/filter_langids.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

sffc
sffc previously approved these changes Sep 20, 2020
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/locale/examples/filter_langids.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@zbraniecki zbraniecki requested a review from sffc September 20, 2020 18:58
sffc
sffc previously approved these changes Sep 21, 2020
.collect();

// 2. Filter for LanguageIdentifiers with Language subtag `en`.
let en_lang: subtags::Language = "en".parse().expect("Failed to parse language subtag.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a good place to demo the macros?

Copy link
Member Author

Choose a reason for hiding this comment

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

once we land them, I'll switch to them in examples :)

filmil
filmil previously approved these changes Sep 21, 2020
echeran
echeran previously approved these changes Sep 21, 2020
Copy link
Contributor

@echeran echeran left a comment

Choose a reason for hiding this comment

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

LGTM. Had a minor Rust question, but it is unrelated to the example code.

fn main() {
let args: Vec<String> = env::args().collect();

let (_input, _output) = if let Some(input) = args.get(1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional/minor Rust question: Would it have been possible for this to be written as:

    let _input = if let Some(input) = args.get(1) {
        input.as_str()
    } else {
        DEFAULT_INPUT
    };
    let _ouptut = filter_input(&input);
...
    println!("...", &_input, &_output);

I forgot if println! complains on &String or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it! Applied! thank you!

@zbraniecki zbraniecki dismissed stale reviews from echeran, filmil, and sffc via a4b6b00 September 21, 2020 20:51
nciric
nciric previously approved these changes Sep 21, 2020
Copy link
Contributor

@nciric nciric left a comment

Choose a reason for hiding this comment

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

Optional: Note somewhere that this is not the best way to do locale matching - use LanguageMatcher (TBD) instead.

@zbraniecki zbraniecki merged commit 1f08acc into unicode-org:master Sep 22, 2020
@zbraniecki zbraniecki deleted the locale-examples branch October 19, 2020 15:49
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.

6 participants