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? |
| @@ -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.
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.
@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.
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.
@aweigold if the client passes only new Locale("en") is must return only "en"?
There was a problem hiding this comment.
So it's working correctly
There was a problem hiding this comment.
@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.
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('_', '-'))); | |||
|
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