-
Notifications
You must be signed in to change notification settings - Fork 28
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
Adding Locale object as parameter to DeliveryParameterBuilder #30
Conversation
@aweigold I getting an error with the build on travis. Do you know what it is? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm if the Kentico API will accept a language with an underscore between the language and variant instead of a hyphen, if it does not, this method will need to do that String manipulation as toString() on a Locale formats it with an underscore.
@@ -179,6 +180,13 @@ public DeliveryParameterBuilder language(String language) { | |||
return this; | |||
} | |||
|
|||
public DeliveryParameterBuilder language(Locale language) { | |||
if (language != null) { | |||
nameValuePairs.add(new BasicNameValuePair(LANGUAGE, language.toString())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not work. Although you can manually make a locale in the "-" syntax, you are actually only setting the language doing so. Constructing language with a variant with a Java locale is done via one of the following examples:
Locale.UK
new Locale("en", "GB")
The toString() method on Locale will return a locale formatted as "_", which doesn't match the spec from the Kentico API. Unless that format is supported, this method will need to do the proper string formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aweigold so new Locale("en", "GB")
must return en-GB
?
If this is the case, I can replace the "_" for "-"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can work. You will also want to unit test an edge case where no variant is set, so if just new Locale("en") is passed, you are not sending in "en-".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aweigold if the client passes only new Locale("en")
is must return only "en"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's working correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cunhazera "So it's working correctly" as in the Kentico API will accept the underscores? Or it's working correctly if you just replace the "_" for "-" ? If the latter, that replacement will work, I was just advising that it would make sense to do a unit test for that case as well.
@@ -272,8 +272,17 @@ public void testLanguage() { | |||
} | |||
|
|||
@Test | |||
public void testLocaleLanguage() { | |||
List<NameValuePair> params = DeliveryParameterBuilder.params().language(new Locale("pt-br")).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is only setting the language "pt-br" instead of the language "pt" with variant "br".
The PR build issue is fixed with #31 |
@@ -182,7 +182,7 @@ public DeliveryParameterBuilder language(String language) { | |||
|
|||
public DeliveryParameterBuilder language(Locale language) { | |||
if (language != null) { | |||
nameValuePairs.add(new BasicNameValuePair(LANGUAGE, language.toString())); | |||
nameValuePairs.add(new BasicNameValuePair(LANGUAGE, language.toString().replace('_', '-'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Looks like I spoke too soon on the PR fix... this is good to merge in. Thanks! |
@aweigold thank you, man. |
thank you @cunhazera and @aweigold ! |
Hi @cunhazera, |
Hello @petrsvihlik I just sent an email with my address. Thank you so much! |
Solve this: #28