[android] "collator" and "resolved-locale" expressions support #12688
Conversation
LukasPaczos
force-pushed
the
12268-android-collator-wrapper
branch
2 times, most recently
from
0cfbb62
to
cdefd7f
Compare
| * @return expression | ||
| * @see <a href="https://www.mapbox.com/mapbox-gl-js/style-spec/#expressions-!=">Style specification</a> | ||
| */ | ||
| public static Expression neq(Expression compareOne, boolean compareTwo, @NonNull Expression collator) { |
There was a problem hiding this comment.
do we need collator support for boolean comparison? Don't think collator is used in core when doing such a comparison. I think String only suffices.
@ChrisLoer can you confirm?
There was a problem hiding this comment.
Yup, we only have collator-supporting overloads for the string comparisons -- so the only way you'd include a boolean in a collator comparison is if an automatic bool->string coercion had taken place first.
There was a problem hiding this comment.
2164485 removes unnecessary boolean and Number overloads.
| * @return expression | ||
| * @see <a href="https://www.mapbox.com/mapbox-gl-js/style-spec/#expressions-resolved-locale">Style specification</a> | ||
| */ | ||
| public static Expression locale(Expression collator) { |
There was a problem hiding this comment.
convention so far has been to concatenate the style-spec syntax, so resolved-locale would be resolvedLocale vs locale. Haven't got any strong arguments not using locale in this case though.
| * @return expression | ||
| * @see <a href="https://www.mapbox.com/mapbox-gl-js/style-spec/#expressions-types-collator">Style specification</a> | ||
| */ | ||
| public static Expression collator(boolean caseSensitive, boolean diacriticSensitive, String locale) { |
There was a problem hiding this comment.
no actionable for this PR, but would a variant of Locale.java make sense? atm it's unclear to me as a developer which String locale I should provide is it a the full name? or the a ISO 639-1 code? By exposing a Locale.java variant we can relieve the developer of that burden and provide the correct value to core ourselves.
There was a problem hiding this comment.
This is a good question, however, we can still make it easier by taking Locale as the parameter and building the tag ourselves.
There was a problem hiding this comment.
The string is a "BCP 47 language tag" and I think the only argument for using a string type is consistency with what's supported in the style spec (e.g. a style might specify extensions in the BCP 47 tag that can't be represented by a Java Locale). But in general using the Java type seems more ergonomic to me here.
There was a problem hiding this comment.
c577372 I opted for building the tag from the passed Locale object manually. Fwiw, users can still build language tags manually with Expression#literal.
|
thanks for fixing all the existing javadoc issues! |
There was a problem hiding this comment.
This looks good to me. 👍
Do we maintain Expression.java by hand? It's huge!
I don't know enough about Java dev to even frame this question correctly, but is there something like a JsonObjectBuilder we could use instead of hand-building JSON with a StringBuilder?
LukasPaczos
force-pushed
the
12268-android-collator-wrapper
branch
from
cdefd7f
to
c577372
Compare
LukasPaczos
force-pushed
the
12268-android-collator-wrapper
branch
from
c577372
to
2164485
Compare
|
Yes @ChrisLoer, the most popular tool is Gson and we are partially using it to deserialize the raw JSON string. However, it would be kinda hard to set it up to handle the little details that our |
Closes #12268 by adding
"collator"and"resolved-locale"expressions support.This PR also adds an
ExpressionMapclass that can be used to parse future expression that are represented as maps/dictionaries/JsonObjects.