Remove domain models #41
Conversation
Using the track with typed events is no longer supported
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 6 6
Lines 117 144 +27
Branches 19 27 +8
======================================
- Misses 117 144 +27
Continue to review full report at Codecov.
|
| @@ -1 +1 @@ | |||
| version=0.4.2 | |||
| version=0.5.0 | |||
There was a problem hiding this comment.
Is this change necessary?
There was a problem hiding this comment.
With all the breaking changes introduced in this PR and #39, I thought it was better to bump the minor version.
There was a problem hiding this comment.
Yes, makes sense, I was thinking that was targetting a different thing. Ignore me.
velocidi-sdk/src/test/kotlin/com/velocidi/TrackingEventsTest.kt
Outdated
Show resolved
Hide resolved
| val k = if (path.isEmpty()) idx else "[$idx]" | ||
| toQueryParamsAux(elem.opt(idx), qs, path + k) | ||
| } | ||
| is Boolean, is Int, is Byte, is Char, is String, is Double, is Float, is Long, is Short -> |
There was a problem hiding this comment.
Is this when block exhaustive? If not, can't we just have a else or default case here?
There was a problem hiding this comment.
It is not. Unmatched types, like Null will not be serialized. In any case, I added an "else case" in 76c5c12 to make this behavior more explicit
There was a problem hiding this comment.
What do you think of something like this:
when (elem) {
JSONObject.NULL, null ->
{}
is JSONObject ->
for (key in elem.keys()) {
val pathKey = if (path.isEmpty()) key else "[$key]"
toQueryParamsAux(elem[key], qs, path + pathKey)
}
is JSONArray ->
for (idx in 0..elem.length()) {
toQueryParamsAux(elem.opt(idx), qs, "$path[$idx]")
}
else ->
qs[path] = elem.toString()
}
}There was a problem hiding this comment.
That change will make us serialize non-primitive types and we might get some weird results. I added an example in 15d19b8 to make this clearer. Using the implementation above Foo would be serialized into foo=Foo(bar=bar), which is not a valid structure for the CDP.
|
|
||
| assertThat(event).isEqualTo(eventObj.toQueryParams()) | ||
| assertThat(event).containsAllEntriesOf(eventObj.toQueryParams()) |
There was a problem hiding this comment.
What does this mean, that the first map may contain more entries than the second? If so, shouldn't we be using a more strict matcher throughout the file?
There was a problem hiding this comment.
Nice catch. The first map could, potentially, contain more elements than the second. AssertJ (assert library used) doesn't provide a matcher that does what we want so I changed this spec to use a custom matcher everywhere. Check 51ec0d7
There was a problem hiding this comment.
I find it strange we don't have that matcher in the core library. We have containsExactlyEntriesOf which considers order though... I guess this will do.
README.md
Outdated
|
|
||
| Velocidi.getInstance().track(CustomTrackingEventFactory.buildFromJSON(event)) | ||
| //Using a JSONObject | ||
| Velocidi.getInstance().track(UserId("<Advertising ID>", "gaid"), JSONObject(eventJson)) |
There was a problem hiding this comment.
As in the sample app, I would prefer to use the JSONObject as a dictionary, instead of relying on the string parsing.
There was a problem hiding this comment.
Improved in 413f796
velocidi-sample/src/main/java/com/velocidi/sampleapp/MainActivity.kt
Outdated
Show resolved
Hide resolved
…ity.kt Co-authored-by: André Cardoso <thyandrecardoso@hotmail.com>
…roid-sdk into remove-domain-models
Depends on #40
Removes domain models from SDK. This changes the
eventargument in the tracking API to accept a JSON string or JSONObject.