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.

Sign up for GitHub

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

Jump to bottom

Fix maximize function #1171

Merged
merged 5 commits into from
Nov 8, 2021
Merged

Fix maximize function #1171

merged 5 commits into from
Nov 8, 2021

Conversation

pandusonu2
Copy link
Contributor

@pandusonu2 pandusonu2 commented Oct 12, 2021 •
edited

Fixes #817

@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

@pandusonu2
Copy link
Contributor Author

Explaining the changes:

When und_latn_AQ is being tried to maximize:

Current implementation:
Its first checked with script_region, it doesn't find a match.
It is then checked with script, it doesn't find a match
It returns data.und, which is en_Latn_AQ

After the changes:
First checks, script_region, it doesnt find a match
Next checks, script, it doesnt find a match
Later checks, region, finds a match, and returns that.

@pandusonu2
Copy link
Contributor Author

PR Bump

cc: @sffc

dminor
dminor requested changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@dminor dminor left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this!

components/locale_canonicalizer/src/locale_canonicalizer.rs Outdated Show resolved Hide resolved
components/locale_canonicalizer/src/locale_canonicalizer.rs Show resolved Hide resolved
@sffc sffc requested review from Manishearth and removed request for zbraniecki October 19, 2021 15:21
@pandusonu2
Copy link
Contributor Author

PR check ping

Manishearth
Manishearth previously approved these changes Oct 25, 2021
View reviewed changes
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.

code looks okay to me but I'd like @dminor to be the one to give it final approval

components/locale_canonicalizer/src/locale_canonicalizer.rs Outdated Show resolved Hide resolved
@pandusonu2
Copy link
Contributor Author

@dminor PR bump

zbraniecki
zbraniecki reviewed Nov 3, 2021
View reviewed changes
components/locale_canonicalizer/src/locale_canonicalizer.rs Outdated
@@ -186,11 +186,23 @@ fn update_langid(
entry: &LanguageIdentifier,
langid: &mut LanguageIdentifier,
) -> CanonicalizationResult {
let language_unmodified = langid.language.clone();
let script_unmodified = langid.script.clone();
let region_unmodified = langid.region.clone();
Copy link
Member

@zbraniecki zbraniecki Nov 3, 2021 •
edited

Choose a reason for hiding this comment

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

question: wouldn't it be much cheaper to register changes, rather than clone and compare?

I mean:

let language_modified = false;

if (...) {
    language_modified = true;
}

if language_modified {
    CanonicalizationResult::Modified    
} else {
    CanonicalizationResult::Unmodified
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please check the PR now

dminor
dminor previously approved these changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@dminor dminor left a comment

Choose a reason for hiding this comment

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

Sorry about the very long delay in reviewing this. This looks fine to me, but please have a look at Zibi's comments as well.

@sffc
Copy link
Member

sffc commented Nov 5, 2021

@pandusonu2 PTAL at Zibi's question above as well as the failing Clippy lint

@sffc sffc added the waiting-on-author PRs waiting for action from the author for >7 days label Nov 5, 2021
@dminor dminor removed the waiting-on-author PRs waiting for action from the author for >7 days label Nov 8, 2021
dminor
dminor previously approved these changes Nov 8, 2021
View reviewed changes
zbraniecki
zbraniecki previously approved these changes Nov 8, 2021
View reviewed changes
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

lgtm! Minor stylistic nits left.

components/locale_canonicalizer/src/locale_canonicalizer.rs Outdated Show resolved Hide resolved
components/locale_canonicalizer/src/locale_canonicalizer.rs Outdated Show resolved Hide resolved
@pandusonu2 pandusonu2 dismissed stale reviews from zbraniecki and dminor via c9f3d8a November 8, 2021 17:35
zbraniecki
zbraniecki approved these changes Nov 8, 2021
View reviewed changes
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

lgtm!

@pandusonu2
Copy link
Contributor Author

@sffc I dont have permissions to merge it. Please do merge it, after the tests are run

@zbraniecki zbraniecki merged commit 027c190 into unicode-org:main Nov 8, 2021
@pandusonu2 pandusonu2 deleted the maximize-fix branch November 8, 2021 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zbraniecki zbraniecki zbraniecki approved these changes

@dminor dminor dminor left review comments

@Manishearth Manishearth Manishearth left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect likely subtags maximization of und-Latn-AQ
5 participants
@pandusonu2 @sffc @dminor @zbraniecki @Manishearth