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

Make Region, Script, and Variant subtags ULE #1696

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Conversation

sffc
Copy link
Member

@sffc sffc commented Mar 15, 2022

Progress on #831
Depends on #1694

This PR is intended to be non-controversial. I split Language into its own PR #1695 since that one is not already its own ULE.

Note: This PR is intended to be based on top of #1694.

@sffc sffc requested review from zbraniecki, nciric and a team as code owners March 15, 2022 03:31
@sffc sffc requested review from Manishearth and removed request for zbraniecki and nciric March 15, 2022 03:31
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

I'm kinda iffy about having try_from_raw take references, everything else looks great.

components/locid/src/subtags/region.rs Outdated Show resolved Hide resolved
@@ -38,6 +38,7 @@ tinystr = { path = "../../utils/tinystr", version = "0.5.0", features = ["alloc"
serde = { version = "1.0", optional = true, default-features = false, features = ["alloc"] }
writeable = { version = "0.3", path = "../../utils/writeable" }
displaydoc = { version = "0.2.3", default-features = false }
zerovec = { version = "0.6", path = "../../utils/zerovec", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

thought: hmm, this is the first time we're making zerovec optional. I see why — locid doesn't need it, locid's clients do. I feel like given the generally pervasive use of zerovec I'd be open to making this a mandatory dependency, but either works for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's discuss in a separate issue: #1700

components/locid/src/subtags/region.rs Outdated Show resolved Hide resolved
components/locid/src/subtags/region.rs Show resolved Hide resolved
components/locid/src/subtags/region.rs Outdated Show resolved Hide resolved
@dpulls
Copy link

dpulls bot commented Mar 15, 2022

🎉 All dependencies have been resolved !

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • Cargo.lock is different
  • utils/tinystr/Cargo.toml is no longer changed in the branch
  • utils/tinystr/src/ascii.rs is no longer changed in the branch
  • utils/tinystr/src/error.rs is no longer changed in the branch
  • utils/tinystr/src/int_ops.rs is no longer 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!

  • Cargo.lock is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@sffc
Copy link
Member Author

sffc commented Mar 16, 2022

^ These are both rebases; no code changes.

@sffc
Copy link
Member Author

sffc commented Mar 16, 2022

Ready for re-review. The code changes are the last two commits.

@sffc sffc merged commit 35907cb into unicode-org:main Mar 16, 2022
@sffc sffc deleted the subtag-ule branch March 16, 2022 01:06
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.

2 participants