1.0.0 #83

aldemirenes · 2019-07-05T11:51:40Z

No description provided.

aldemirenes · 2019-07-05T11:52:32Z

JSON encoder and decoder for new ParsingError will be added and tests will be updated.

chuwy

Needs further tweaking.

chuwy · 2019-07-05T11:57:24Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

+  case class InvalidValue(key: String, value: String) extends ParsingError
+
+  /**
+    * Represents unexpected errors


https://twitter.com/carterwickstrom/status/1014165500056596481

chuwy · 2019-07-05T11:58:33Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

+    * @param expectedNum expected number of columns
+    * @param gotNum number of given columns
+    */
+  case class ColumnNumberMismatch(expectedNum: Int, gotNum: Int) extends ParsingError


Num is a Hungarian notation. We don't need it with static types.

How many valid values we have for expectedNum?

Only one, however I'd think putting it in there makes sense in order to inform about that.

If there's only one possible value, why would anyone bother about it at all?

I thought that the place where this error is got can want to know how many field is expected. If you think it is useless, I can remove it.

I thought that the place where this error is got can want to know how many field is expected. If you think it is useless, I can remove it.

For me it looks like constant provided by Analytics SDK. If we'll get millions of rows with expectedNum: 131 I don't think anyone would benefit from it.

chuwy · 2019-07-05T12:00:57Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/decode/ValueDecoder.scala

@@ -50,7 +51,7 @@ private[decode] object ValueDecoder {
  implicit final val stringColumnDecoder: ValueDecoder[String] =
    fromFunc[String] {
      case (key, value) =>
-        if (value.isEmpty) (key, s"Field $key cannot be empty").asLeft else value.asRight
+        if (value.isEmpty) InvalidValue(key.toString, s"Field $key cannot be empty").asLeft else value.asRight


Second member of InvalidValue is value, but we add a human-readable message here.

Yeah I think I missed it, will correct it.

chuwy · 2019-07-05T12:02:57Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/decode/ValueDecoder.scala

@@ -105,22 +106,22 @@ private[decode] object ValueDecoder {
          value.toDouble.some.asRight
        } catch {
          case _: NumberFormatException =>
-            (key, s"Cannot parse key $key with value $value into double").asLeft
+            InvalidValue(key.toString, s"Cannot parse key $key with value $value into double").asLeft


It seems that type argument is another common pattern for InvalidValue? double, boolean etc. On the other hand it is deriveable from key...

Ignore above - just thoughts aloud.

chuwy · 2019-07-05T12:04:38Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/decode/ValueDecoder.scala

        else
          try {
-            UUID.fromString(value).asRight[(Key, String)]
+            UUID.fromString(value).asRight[InvalidValue]


InvalidValue is what we call "data constructor" - its better to not have it in a type position. Type is ParsingError, which is "root" of ADT.

chuwy · 2019-07-05T12:04:58Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

+    * @param key key of field
+    * @param value value of the field
+    */
+  case class InvalidValue(key: String, value: String) extends ParsingError


Why not to keep key as Symbol so far? It seems we're stringifying it everywhere.

chuwy · 2019-07-05T12:05:23Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

@@ -0,0 +1,36 @@
+package com.snowplowanalytics.snowplow.analytics.scalasdk


License header.

chuwy · 2019-07-05T12:05:54Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

+    * @param error error message
+    */
+  case class UnexpectedError(error: String) extends ParsingError
+}


Codecs are missing.

Will add them.

chuwy · 2019-07-05T12:06:02Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

+    * @param expectedNum expected number of columns
+    * @param gotNum number of given columns
+    */
+  case class ColumnNumberMismatch(expectedNum: Int, gotNum: Int) extends ParsingError


src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

chuwy

The "Prepare for release" ticket is missing.

I have a small concern about NonEmptyList of errors. If I missing something and it is possible to have it then let's go ahead and release it today.

chuwy · 2019-07-25T10:57:48Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/ParsingError.scala

+    * RowDecoder, getting more or less values than expected is impossible
+    * due to type check. Therefore 'UnexpectedError' is returned for
+    * these cases
+    * @param error error message


Can we mention that it usually can be ignored as "not possible"

chuwy · 2019-07-25T11:00:49Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/decode/Parser.scala

-    val decoded = decoder(zipped)
-    decoded.map { decodedValue => generic.from(decodedValue) }
+    if (values.length == 1) {
+      Validated.Invalid(NonEmptyList.of(NonTSVPayload))


For me it looks that in fact or error type is:

NonTSVPayload | ColumnNumberMismatch | NonEmptyList[InvalidValue]

And not

NonEmptyList[NonTSVPayload] | NonEmptyList[ColumnNumberMismatch] | NonEmptyList[InvalidValue]

I.e. we should never check a case of NonEmptyList.of(NonTSVPayload, ColumnNumberMismatch(_))

Am I missing something?

chuwy

👍 @aldemirenes. Could you please also implement fresh tickets I added to 1.0.0 milestone.

chuwy · 2019-08-13T14:11:20Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/Event.scala

+    * Creates empty event which optional fields of it are None
+    * Only necessary fields are given as arguments
+    */
+  def emptyEvent(id: UUID, collectorTstamp: Instant, vCollector: String, vTstamp: String): Event =


What is vTstamp?

It is v_etl. I think it is better to change its name to vEtl.

chuwy · 2019-08-16T06:41:38Z

project/ScoverageSettings.scala

+
+object ScoverageSettings {
+
+  val settings = Seq(


Can we move it into BuildSettings.scala as scoverageSettings?

chuwy · 2019-08-16T06:42:43Z

project/MimaSettings.scala

+import sbt.Keys._
+import sbt._
+
+object MimaSettings {


Can it be in BuildSettings?

chuwy · 2019-08-16T06:44:43Z

project/MimaSettings.scala

+  // clear-out mimaBinaryIssueFilters and mimaPreviousVersions.
+  // Otherwise, add previous version to set without
+  // removing other versions.
+  val mimaPreviousVersions = Set()


So, we'll have to add it manually after every release? Is there a way to automate it? Otherwise we'll need to have a standardized place for "Release checklist"? Wiki? Repo itself? (IMO its related to DQA, @dilyand)

I thought and looked for a way to automate but there is no clear way for this as far as I understand. One of the reason is that these previous versions should be determined by the breaking changes in the current version. So, let's say we bumped from 1.0.0 to 1.0.1 and there is no breaking changes obviously, in that case 1.0.0 should be added to mimaPreviousVersions. However, in case of bumping from 1.0.1 to 2.0.0 which most probably there will be breaking changes, mimaPreviousVersions should be cleared-out. So, maybe some scripts can be written for checking current version and place previous versions according to that but I could not be sure about it.

Yeah, I think we just should have a "Release checklist" wikii page somehwere, because people will keep forgetting about it.

chuwy · 2019-08-16T06:46:45Z

src/main/scala/com.snowplowanalytics.snowplow.analytics.scalasdk/RunManifests.scala

-  * @param dynamodb AWS DynamoDB client
-  * @param tableName existing DynamoDB table name with run manifests
-  */
-class RunManifests(dynamodb: AmazonDynamoDB, tableName: String) {


I was thinking more about https://www.scala-lang.org/api/current/scala/deprecated.html rather than deleting it compltely. It just unties our hands about deleting it in 1.x series, i.e. its not something users should rely on anyways.

So, should I make it deprecated or leave it as removed ?

deprecated

chuwy · 2019-08-16T06:47:35Z

@aldemirenes could you please also rename the branch to release/1.0.0 (and probably open a new PR)?

aldemirenes · 2019-08-19T08:05:16Z

Closed in favor of #89

chuwy suggested changes Jul 5, 2019

View reviewed changes

aldemirenes force-pushed the release/0.5.0 branch from 89af657 to 1ba3bd8 Compare July 8, 2019 06:52

chuwy reviewed Jul 25, 2019

View reviewed changes

aldemirenes force-pushed the event-decoder branch from e02f11f to 8e27172 Compare August 6, 2019 12:20

aldemirenes force-pushed the release/0.5.0 branch 2 times, most recently from 5fcd769 to ea5d71e Compare August 13, 2019 10:30

aldemirenes changed the base branch from event-decoder to master August 13, 2019 10:30

aldemirenes force-pushed the release/0.5.0 branch from ea5d71e to bd30158 Compare August 13, 2019 10:32

aldemirenes changed the title ~~0.5.0~~ 1.0.0 Aug 13, 2019

chuwy self-requested a review August 13, 2019 14:10

chuwy reviewed Aug 13, 2019

View reviewed changes

aldemirenes force-pushed the release/0.5.0 branch 2 times, most recently from 6957f7a to c110d6d Compare August 14, 2019 20:12

aldemirenes added 4 commits August 16, 2019 00:11

Make parsing errors type-safe (closes #75)

84fb81d

Add function for creating empty event (closes #81)

74476ad

Remove Vagrant setup (closes #84)

fb7c576

aldemirenes force-pushed the release/0.5.0 branch from e7641e0 to 34cdaa0 Compare August 15, 2019 21:18

chuwy reviewed Aug 16, 2019

View reviewed changes

aldemirenes force-pushed the release/0.5.0 branch 2 times, most recently from a03ce7b to 6afbb62 Compare August 17, 2019 20:24

aldemirenes added 4 commits August 17, 2019 23:30

Deprecate run manifest (closes #86)

8238fd2

Integrate MiMa (closes #87)

f5a2cf7

Prepared for release

fe43aaf

M1

fe42bf8

Integrate scoverage

0378a73

aldemirenes force-pushed the release/0.5.0 branch from 6afbb62 to 6818293 Compare August 17, 2019 20:40

Integrate sbt-gh-pages to create GH Pages from Scaladoc

752f4a4

aldemirenes force-pushed the release/0.5.0 branch from 6818293 to 752f4a4 Compare August 19, 2019 08:02

aldemirenes closed this Aug 19, 2019

aldemirenes deleted the release/0.5.0 branch August 19, 2019 08:06

		@@ -0,0 +1,36 @@
		package com.snowplowanalytics.snowplow.analytics.scalasdk

1.0.0 #83

1.0.0 #83

Conversation

aldemirenes commented Jul 5, 2019

aldemirenes commented Jul 5, 2019

chuwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuwy commented Aug 16, 2019

aldemirenes commented Aug 19, 2019