-
Notifications
You must be signed in to change notification settings - Fork 131
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
Add support for promoteId #636
Conversation
extension-style/src/main/java/com/mapbox/maps/extension/style/types/PromoteId.kt
Outdated
Show resolved
Hide resolved
a47f803
to
343c68f
Compare
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.
LGTM
*/ | ||
@Keep | ||
data class PromoteId @JvmOverloads constructor( | ||
val sourceId: String? = null, |
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.
if this param is optional - shouldn't it be actually second one? guess it will make usage easier (e.g. not use named arguments below in static method)
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.
i did it the same way initially, but then aligned it with the docs here
https://docs.mapbox.com/mapbox-gl-js/style-spec/sources/#vector-promoteId.
Either a property name, or an object of the form {<sourceLayer>: <propertyName>}
But definitely its better to use optional param as last args.
extension-style/src/main/java/com/mapbox/maps/extension/style/types/PromoteId.kt
Show resolved
Hide resolved
} | ||
is HashMap<*, *> -> { | ||
@Suppress("UNCHECKED_CAST") | ||
val propertyMap = propertyName as HashMap<String, String> |
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.
what if it's not HashMap<String, String>
? Will we have runtime crash? Perhaps use try-catch here to produce better exception text
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.
I'm not sure how it works, could check like:
is HashMap<String, String> -> {
instead of
is HashMap<*, *> -> {
work here?
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.
Using HashMap<String,String> gives compile time error.
Cannot check for instance of erased type: kotlin.collections.HashMap<String, String> /* = java.util.HashMap<String, String>
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.
I will add more try{} catch{} for hashmap, but the args for this function returned from core in either string or map<string,string>.
extension-style/src/main/java/com/mapbox/maps/extension/style/types/PromoteId.kt
Outdated
Show resolved
Hide resolved
val sourceId: String? = null, | ||
val propertyName: String | ||
) { | ||
internal fun toValue(): Value { |
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.
Is it used in tests only?
if yes - do we need this function at all here? we could add it as extension function in tests or do smth similar
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.
it also used in Builder methods of source in order to add the properties to source.
* @param sourceId source layer id of the feature, either source [GeoJsonSource] or [VectorSource]. | ||
* @param propertyName feature property name. | ||
* | ||
* For more information see [The online documentation]https://docs.mapbox.com/mapbox-gl-js/style-spec/types/#promoteId). |
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.
* For more information see [The online documentation]https://docs.mapbox.com/mapbox-gl-js/style-spec/types/#promoteId). | |
* For more information see [The online documentation](https://docs.mapbox.com/mapbox-gl-js/style-spec/types/#promoteId). |
And also not sure this will work as expected in real generated docs
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.
Also link does not lead me to promote id...
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.
Yeah, the link is broken from gl-js side, I have asked @alexshalamov if that will be fixed or i will remove from here :)
/** | ||
* Holds a property type to promote a specific feature for feature state API. | ||
* | ||
* @param sourceId source layer id of the feature, either source [GeoJsonSource] or [VectorSource]. |
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.
Do we have a check for that?
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.
promoteId
is only available for GeoJson or vector source on builder method so we don't need to check here.
But if you are asking that if we initialize promoteId with the wrong sourceId and provide that to geojson builder -> IMO it should be handled by core itself, and shouldn't be applied to feature state, but I will test it.
is HashMap<*, *> -> { | ||
@Suppress("UNCHECKED_CAST") | ||
val propertyMap = propertyName as HashMap<String, String> | ||
val key = propertyMap.keys.iterator().next() |
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.
what if we pass an empty hash map? 😄
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.
fair point, will add a check here. :D
companion object { | ||
/** | ||
* Construct a [PromoteId] object from a Property returned from the core. | ||
* Can be either of type [String] or [HashMap] of <String,String> |
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.
Specify @throws
as well as @param
* Construct a [PromoteId] object from a Property returned from the core. | ||
* Can be either of type [String] or [HashMap] of <String,String> | ||
*/ | ||
fun fromProperty(propertyName: Any) = when (propertyName) { |
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.
Could you add tests for this method and cover all scenarios?
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.
I think we could split this function to two functions to make the API signature strongly typed, since we only support two types of data. i.e. fromProperty(propertyName: String)
and fromProperty(propertyMap: Map<String, String>)
.
Thinking again, why would we need to have this method? The promoteId could either a property name, or an object of the form {: }, so the constructor should be well enough for this purpose. cc @ank27
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.
If it's purely used to deserialise from a Value object, I think it's better to use fun fromValue(value: Value)
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.
and nit: could be internal.
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.
I think we could split this function to two functions to make the API signature strongly typed, since we only support two types of data. i.e.
fromProperty(propertyName: String)
andfromProperty(propertyMap: Map<String, String>)
.Thinking again, why would we need to have this method? The promoteId could either a property name, or an object of the form {: }, so the constructor should be well enough for this purpose. cc @ank27
It was made this way to handle the errors in case core provides other type formats.
hmm, I see that we can use Value
as it is already wrapper for type.
fd16ddb
to
977719e
Compare
PromoteId.fromProperty(hashMapOf(1 to 2)) | ||
} | ||
|
||
internal data class MockType(val a: String, val b: String) |
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.
internal data class MockType(val a: String, val b: String) | |
private data class MockType(val a: String, val b: String) |
throw RuntimeException("$propertyName must be in the format HashMap<String,String>") | ||
} | ||
} | ||
else -> throw RuntimeException("Wrapping ${propertyName::class.java.simpleName} to PromoteId is not supported.") |
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.
else -> throw RuntimeException("Wrapping ${propertyName::class.java.simpleName} to PromoteId is not supported.") | |
else -> throw UnsupportedTypeException("Wrapping ${propertyName::class.java.simpleName} to PromoteId is not supported.") |
import org.junit.runner.RunWith | ||
import org.robolectric.RobolectricTestRunner | ||
|
||
@RunWith(RobolectricTestRunner::class) |
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.
do really need this runner?
|
||
@Test | ||
fun fromProperty_HashMap() { | ||
val expected = PromoteId("abc", "source") |
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.
nice to add one more test PromoteId("abc", null)
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.
and PromoteId("", "source")
977719e
to
4c5bebd
Compare
4c5bebd
to
65b295c
Compare
throw RuntimeException("$propertyName must be in the format HashMap<String,String>") | ||
} | ||
} | ||
else -> throw UnsupportedOperationException("Wrapping ${propertyName::class.java.simpleName} to PromoteId is not supported.") |
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.
can we somehow check this exception?
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.
added tests for this
65b295c
to
5424fc3
Compare
5424fc3
to
8cb2b41
Compare
48c13ce
to
fe59c97
Compare
* source, the same property is used across all its source layers. | ||
*/ | ||
fun promoteId(value: PromoteId) = apply { | ||
val propertyValue = PropertyValue("promoteId", TypeUtils.wrapToValue(value.toValue())) |
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.
Nit: do we still need wrapToValue since toValue() should have been Value type already?
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.
LGTM with a nit
fe59c97
to
0496529
Compare
PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >
Pull request checklist:
mapbox-maps-android
changelog:<changelog>Add support for PromoteId to be used with Feature state API.</changelog>
.Summary of changes
This PR adds support for
promoteId
which allows a developer to promote a particular string / source-property to be the feature state id in features.promoteId
is a feature for geojson and vector sources. The feature allows to promote feature's property to a feature id, so that promoted id can be used with FeatureState API.