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 icu-locale macros #220

Merged
merged 4 commits into from
Oct 8, 2020
Merged

Conversation

zbraniecki
Copy link
Member

Fixes #75.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/locale/macros/src/lib.rs is different
  • components/locale/macros/tests/macros.rs is different
  • components/locale/src/subtags/region.rs is different
  • components/locale/src/subtags/script.rs is different
  • components/locale/src/subtags/variant.rs is now changed in the branch

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/macros/src/lib.rs is different
  • components/locale/macros/tests/macros.rs is different
  • components/locale/src/parser/langid.rs is now changed in the branch
  • components/locale/src/subtags/language.rs is different
  • components/locale/src/subtags/region.rs is different
  • components/locale/src/subtags/script.rs is different
  • components/locale/src/subtags/variant.rs is different
  • components/locale/src/subtags/variants.rs is now changed in the branch

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/src/parser/langid.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@zbraniecki zbraniecki marked this pull request as ready for review September 3, 2020 19:14
@zbraniecki zbraniecki requested review from nciric and a team as code owners September 3, 2020 19:14
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@zbraniecki
Copy link
Member Author

@echeran I may need your help in getting the actions cache invalidated - the failures are because we use outdated Rust. We should bump to 1.46 which will happen on cache regeneration.

The rest is ready for review!

I had to wrap Variants in Option to allow it to be empty from const fn so that langid! macro works as const. I think it's a small complication that allows for a major usability improvement and it is an internal change that can be safely cleaned up once Rust gains support for Box::new([]) in const fn.

The rest is fairly simple I hope :)

@echeran
Copy link
Contributor

echeran commented Sep 3, 2020

I created an issue to separate out the discussion of the cached artifacts causing conflicts - #225

components/locale/macros/src/lib.rs Show resolved Hide resolved
components/locale/macros/src/lib.rs Show resolved Hide resolved
components/locale/src/subtags/language.rs Outdated Show resolved Hide resolved
components/locale/src/subtags/variants.rs Show resolved Hide resolved
components/locale/macros/src/lib.rs Outdated Show resolved Hide resolved
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/locale/macros/src/lib.rs is different
  • components/locale/macros/tests/macros.rs is different
  • components/locale/src/subtags/variants.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@zbraniecki
Copy link
Member Author

I rebased on top of master, added doc comments and improved handling of variants in macro.

This PR is pending release of Rust 1.47 on October 8th (https://forge.rust-lang.org/), but I'd like to get it reviewed and ready for landing prior to that.

@zbraniecki zbraniecki added the waiting-on-reviewer PRs waiting for action from the reviewer for >7 days label Sep 17, 2020
@zbraniecki zbraniecki requested a review from sffc September 17, 2020 17:07
@sffc sffc removed the waiting-on-reviewer PRs waiting for action from the reviewer for >7 days label Sep 18, 2020
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/locale/macros/Cargo.toml is different
  • components/locale/macros/src/lib.rs is different
  • components/locale/macros/src/token_stream.rs is now changed in the branch
  • components/locale/macros/tests/macros.rs is different
  • components/locale/src/subtags/language.rs is different
  • components/locale/src/subtags/region.rs is different
  • components/locale/src/subtags/script.rs is different
  • components/locale/src/subtags/variant.rs is different
  • components/locale/src/subtags/variants.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@zbraniecki
Copy link
Member Author

Here and elsewhere (this might be a pre-existing issue): I would avoid using the term "perform canonicalization". UTS 35 defines two types of canonicalization:

I applied your feedback to docs, except of Locale and LanguageIdentifier canonicalize method doc which already stated that we perform "all available levels of canonicalization".

I generally believe that cases where anyone cares about some syntax canonicalization but not form are so extremely rare, that I would like to introduce form canonicalization as an implicit "upgrade" to the functionality without changes to the signature and considered non-breaking.

Since it requires data, we may end up having it behind a feature, in which case user can in theory use that feature to decide if the canonicalization will be syntax only or also form, but I'd like to treat the public API as "we will do everything we can do give you a good output" and if we see a body of need accruing for more specificity, we can then break the API and add options to flip parts of canonicalization.

I updated the docs but I'd like to remove the specifier "syntax" once we land the form canonicalization.

Either way, that's a conversation we can have when we work on form canonicalization and I hope this update unblocks this PR.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/locale/src/subtags/language.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

sffc
sffc previously approved these changes Sep 20, 2020
components/locale/src/subtags/language.rs Show resolved Hide resolved
components/locale/macros/src/token_stream.rs Show resolved Hide resolved
components/locale/macros/src/token_stream.rs Show resolved Hide resolved
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • Cargo.toml 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/macros/src/token_stream.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

use proc_macro::TokenStream;
use token_stream::IntoTokenStream;

fn get_value_from_token_stream(input: TokenStream) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

Looking for literal " is also what you do in tinystr. We need to fix this, but it shouldn't block this PR. zbraniecki/tinystr#24

@zbraniecki zbraniecki merged commit 1eae256 into unicode-org:master Oct 8, 2020
@zbraniecki zbraniecki deleted the locale-unchecked 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.

Add macros for locale/langid
5 participants