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

Accumulate case class mapping errors instead of fail-fast #124

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

geoffjohn11
Copy link
Contributor

No description provided.

Copy link
Contributor

@BalmungSan BalmungSan left a comment

Choose a reason for hiding this comment

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

A couple of suggestions. But, in general great work!

I would like to explore if it would be possible to avoid the noise of adding all those errors everywhere, maybe only the HList mapper should be checking if the previous exception was a MultipleIncoercibleException and if so, add a new one?
Or, you may convenience me that it may be useful for other types to report multiple errors.

Comment on lines 155 to 159
decodedHead match{
case Left(th) =>
tail.to(convertedValue, typeHint, th +: errors).map(t => labelled.field[K](null.asInstanceOf[H]) :: t)
case Right(v) => tail.to(convertedValue, typeHint, errors).map(t => labelled.field[K](v) :: t)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would format it like:

decodedHead match {
  case Left(th) =>
     tail.to(convertedValue, typeHint, th :: errors).map(t => labelled.field[K](null.asInstanceOf[H]) :: t)

  case Right(v) =>
    tail.to(convertedValue, typeHint, errors).map(t => labelled.field[K](v) :: t)
}

Also, both null and asInstanceOf are code smells.
There should be a better way to make it typecheck.


(I will try to play with the code on an editor to understand why you did that, but that will probably take me a couple of days)

@@ -13,7 +13,7 @@ import scala.util.Try
object mappers {
@annotation.implicitNotFound("Could not find the ResultMapper for ${A}")
trait ResultMapper[A] { self =>
def to(value: List[(String, Value)], typeHint: Option[TypeHint]): Either[Throwable, A]
def to(value: List[(String, Value)], typeHint: Option[TypeHint], errors: Seq[Throwable] = Nil): Either[Throwable, A]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that default value is not needed at all.
Apply to all the following ones.

core/src/test/scala/neotypes/CompositeTypesSpec.scala Outdated Show resolved Hide resolved
core/src/test/scala/neotypes/CompositeTypesSpec.scala Outdated Show resolved Hide resolved
@geoffjohn11 geoffjohn11 force-pushed the accumulateExceptions branch 2 times, most recently from d133796 to 3e57862 Compare May 5, 2020 20:34
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.

None yet

2 participants