Skip to content

fix(3259): Support deserialization of Locales created using BCP 47 format #3265

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

Merged

Conversation

praneethjreddy
Copy link
Contributor

Fix to support deserialization of Locales created using BCP 47 format

Issue: #3259

@praneethjreddy
Copy link
Contributor Author

@cowtowncoder , Can you take a look at the PR and let me know if any changes needed

@cowtowncoder
Copy link
Member

Thank you @praneethjreddy! I hope to review this soon, hopefully tomorrow.

Aside from reviewing, assuming all goes well one thing I'd need before merging is filled CLA from here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

This only needs to be done once; the usual way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.
Once this is received I can merge the PR (and any future ones you may submit as well!).

Thank you in advance; looking forward to getting this merged and include in 2.13.0!

}

private boolean _isScriptOrExtensionPresent(String value) {
return value.contains("_#");
Copy link
Member

Choose a reason for hiding this comment

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

Is this separator/marker documented somewhere? Would be great to have a link to explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cowtowncoder
Copy link
Member

Looks good: all I need now is the CLA!

@praneethjreddy
Copy link
Contributor Author

Sounds good. I will email CLA today

@cowtowncoder
Copy link
Member

CLA received, can proceed when I have time to do the final review, merge.

@cowtowncoder cowtowncoder merged commit 1e7c1e7 into FasterXML:2.13 Sep 8, 2021
@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 8, 2021

Merged for 2.13, where things are fine. But with 3.0 (master) we'll run into issue due to fix #1600...
Serialization of Locale in 3.0 works as:

                Locale loc = (Locale) value;
                if (loc == Locale.ROOT) {
                    str = "";
                } else {
                    str = loc.toLanguageTag();
                }

instead of relying on Locale.toString().

Not sure what to do about this...

@cowtowncoder
Copy link
Member

@praneethjreddy I was wondering if you might be able to help here, wrt 3.0. Tests are failing, but I am not yet sure if it is false positive (verification assumes old serialization), or if deserialization for 3.0 needs to account for possible variations.
3.0 can be built from master branch (default branch is 2.13, currently).

@praneethjreddy
Copy link
Contributor Author

@cowtowncoder I can take a look. My suggestion is to use fromLanguageTag for deserialization in 3.0 since we are moving away from toString to toLanguageTag for serialization

@cowtowncoder
Copy link
Member

Thank you @praneethjreddy I can try that when I get a chance, to see if just doing that would get past the issues.

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