Add support for promoteId by ank27 · Pull Request #636 · mapbox/mapbox-maps-android

ank27 · 2021-09-13T09:23:47Z

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:

Briefly describe the changes in this PR.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality. If tests were not written, please explain why.
Add example if relevant.
Document any changes to public APIs.
Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
Add an entry inside this element for inclusion in the 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.

Chaoba

LGTM

kiryldz · 2021-09-14T14:44:06Z

+ */
+@Keep
+data class PromoteId @JvmOverloads constructor(
+  val sourceId: String? = null,


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)

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.

kiryldz · 2021-09-14T14:47:08Z

+        }
+        is HashMap<*, *> -> {
+          @Suppress("UNCHECKED_CAST")
+          val propertyMap = propertyName as HashMap<String, String>


what if it's not HashMap<String, String>? Will we have runtime crash? Perhaps use try-catch here to produce better exception text

I'm not sure how it works, could check like:

is HashMap<String, String> -> {

instead of

is HashMap<*, *> -> {

work here?

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>

I will add more try{} catch{} for hashmap, but the args for this function returned from core in either string or map<string,string>.

kiryldz · 2021-09-14T14:50:43Z

+  val sourceId: String? = null,
+  val propertyName: String
+) {
+  internal fun toValue(): Value {


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

it also used in Builder methods of source in order to add the properties to source.

kiryldz · 2021-09-14T14:51:53Z

+ * @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).


Suggested change

* 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

Also link does not lead me to promote id...

Yeah, the link is broken from gl-js side, I have asked @alexshalamov if that will be fixed or i will remove from here :)

kiryldz · 2021-09-14T14:55:04Z

+/**
+ * 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].


Do we have a check for that?

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.

kiryldz · 2021-09-14T14:56:49Z

+        is HashMap<*, *> -> {
+          @Suppress("UNCHECKED_CAST")
+          val propertyMap = propertyName as HashMap<String, String>
+          val key = propertyMap.keys.iterator().next()


what if we pass an empty hash map? 😄

fair point, will add a check here. :D

kiryldz · 2021-09-14T14:59:19Z

+  companion object {
+    /**
+     * Construct a [PromoteId] object from a Property returned from the core.
+     * Can be either of type [String] or [HashMap] of <String,String>


Specify @throws as well as @param

alexander-kulikovskii · 2021-09-15T06:08:57Z

+     * 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) {


Could you add tests for this method and cover all scenarios?

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

If it's purely used to deserialise from a Value object, I think it's better to use fun fromValue(value: Value)

and nit: could be internal.

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

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.

alexander-kulikovskii · 2021-09-17T08:03:58Z

+    PromoteId.fromProperty(hashMapOf(1 to 2))
+  }
+
+  internal data class MockType(val a: String, val b: String)


Suggested change

internal data class MockType(val a: String, val b: String)

private data class MockType(val a: String, val b: String)

alexander-kulikovskii · 2021-09-17T08:05:24Z

+          throw RuntimeException("$propertyName must be in the format HashMap<String,String>")
+        }
+      }
+      else -> throw RuntimeException("Wrapping ${propertyName::class.java.simpleName} to PromoteId is not supported.")


Suggested change

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.")

alexander-kulikovskii · 2021-09-17T08:06:27Z

+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+
+@RunWith(RobolectricTestRunner::class)


do really need this runner?

alexander-kulikovskii · 2021-09-17T08:08:19Z

+
+  @Test
+  fun fromProperty_HashMap() {
+    val expected = PromoteId("abc", "source")


nice to add one more test PromoteId("abc", null)

and PromoteId("", "source")

alexander-kulikovskii · 2021-09-20T11:31:51Z

+          throw RuntimeException("$propertyName must be in the format HashMap<String,String>")
+        }
+      }
+      else -> throw UnsupportedOperationException("Wrapping ${propertyName::class.java.simpleName} to PromoteId is not supported.")


can we somehow check this exception?

added tests for this

pengdev · 2021-09-22T06:49:44Z

+     * source, the same property is used across all its source layers.
+     */
+    fun promoteId(value: PromoteId) = apply {
+      val propertyValue = PropertyValue("promoteId", TypeUtils.wrapToValue(value.toValue()))


Nit: do we still need wrapToValue since toValue() should have been Value type already?

pengdev

LGTM with a nit

ank27 added the feature 🍏 label Sep 13, 2021

ank27 requested a review from a team September 13, 2021 16:52

Chaoba reviewed Sep 14, 2021

View reviewed changes

Comment thread extension-style/src/main/java/com/mapbox/maps/extension/style/types/PromoteId.kt Outdated

ank27 force-pushed the ak-promote-id branch from a47f803 to 343c68f Compare September 14, 2021 08:12

ank27 requested a review from Chaoba September 14, 2021 12:27

Chaoba approved these changes Sep 14, 2021

View reviewed changes

kiryldz reviewed Sep 14, 2021

View reviewed changes

Comment thread extension-style/src/main/java/com/mapbox/maps/extension/style/types/PromoteId.kt

kiryldz reviewed Sep 14, 2021

View reviewed changes

Comment thread extension-style/src/main/java/com/mapbox/maps/extension/style/types/PromoteId.kt Outdated

kiryldz reviewed Sep 14, 2021

View reviewed changes

alexander-kulikovskii reviewed Sep 15, 2021

View reviewed changes

ank27 force-pushed the ak-promote-id branch from fd16ddb to 977719e Compare September 16, 2021 09:22

ank27 requested review from Chaoba, alexander-kulikovskii and kiryldz September 16, 2021 11:33

alexander-kulikovskii reviewed Sep 17, 2021

View reviewed changes

ank27 force-pushed the ak-promote-id branch from 977719e to 4c5bebd Compare September 20, 2021 09:50

ank27 requested a review from alexander-kulikovskii September 20, 2021 09:50

ank27 force-pushed the ak-promote-id branch from 4c5bebd to 65b295c Compare September 20, 2021 10:11

alexander-kulikovskii reviewed Sep 20, 2021

View reviewed changes

ank27 force-pushed the ak-promote-id branch from 65b295c to 5424fc3 Compare September 20, 2021 13:05

ank27 requested a review from alexander-kulikovskii September 20, 2021 13:06

kiryldz approved these changes Sep 21, 2021

View reviewed changes

alexander-kulikovskii approved these changes Sep 21, 2021

View reviewed changes

ank27 force-pushed the ak-promote-id branch from 5424fc3 to 8cb2b41 Compare September 21, 2021 16:37

ank27 force-pushed the ak-promote-id branch 2 times, most recently from 48c13ce to fe59c97 Compare September 21, 2021 17:45

ank27 requested a review from pengdev September 21, 2021 17:46

pengdev reviewed Sep 22, 2021

View reviewed changes

pengdev approved these changes Sep 22, 2021

View reviewed changes

add promoteid

0496529

ank27 force-pushed the ak-promote-id branch from fe59c97 to 0496529 Compare September 22, 2021 07:32

ank27 merged commit b3e6017 into main Sep 22, 2021

ank27 deleted the ak-promote-id branch September 22, 2021 08:14

ank27 self-assigned this Sep 23, 2021

	* 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).

	internal data class MockType(val a: String, val b: String)
	private data class MockType(val a: String, val b: String)

	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.")

Conversation

ank27 commented Sep 13, 2021 • edited by knov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist:

Summary of changes

Uh oh!

Uh oh!

Chaoba left a comment

Choose a reason for hiding this comment

Uh oh!

kiryldz Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ank27 Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pengdev Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pengdev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

ank27 commented Sep 13, 2021 •

edited by knov

Loading

kiryldz Sep 14, 2021 •

edited

Loading

ank27 Sep 14, 2021 •

edited

Loading

pengdev Sep 21, 2021 •

edited

Loading