Skip to content
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

Common: add tests for required property that is explicitly set to nul… #629

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dilyand
Copy link
Contributor

@dilyand dilyand commented Jun 13, 2022

No description provided.

Copy link
Contributor

@benjben benjben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add some context on the issue please ?

@@ -0,0 +1,32 @@
package com.snowplowanalytics.snowplow.enrich.common.enrichments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header missing

Comment on lines 3 to 22
import cats.syntax.option._
import com.snowplowanalytics.snowplow.enrich.common.adapters.RawEvent
import com.snowplowanalytics.snowplow.enrich.common.loaders.CollectorPayload
import com.snowplowanalytics.snowplow.enrich.common.outputs.EnrichedEvent
import com.snowplowanalytics.snowplow.enrich.common.SpecHelpers.MapOps
import org.specs2.mutable.Specification
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we separate packages that come from different artifacts with newlines, and we finish by Snowplow ones

Comment on lines 17 to 26

import com.snowplowanalytics.iglu.core.{SchemaKey, SchemaVer, SelfDescribingData}
import com.snowplowanalytics.iglu.client.ClientError.{ResolutionError, ValidationError}

import com.snowplowanalytics.snowplow.badrows._

import io.circe.Json

import io.circe.parser._
import cats.data.NonEmptyList

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, new lines should stay

@dilyand dilyand force-pushed the test/required-explicit-null-2 branch from 56acddd to c9e3748 Compare June 23, 2022 09:47
@dilyand dilyand force-pushed the test/required-explicit-null-2 branch from c9e3748 to 565bc36 Compare June 23, 2022 09:55
@dilyand
Copy link
Contributor Author

dilyand commented Jun 23, 2022

Addressed comments and rebased on latest develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants