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

Give NonEmptyChain more presence #2431

Merged
merged 2 commits into from
Sep 1, 2018
Merged

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Aug 20, 2018

This adds the ValidatedNec type alias as well as several api enhancements that were previously only available to NonEmptyList

@LukaJCB LukaJCB added this to the 1.3 milestone Aug 20, 2018
@@ -205,6 +205,8 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {
def toValidatedNel(implicit F: Functor[F]): F[ValidatedNel[A, B]] =
F.map(value)(_.toValidatedNel)

def toValidatedNec(implicit F: Functor[F]): F[ValidatedNec[A, B]] = ???
Copy link
Contributor

Choose a reason for hiding this comment

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

implementation coming, I assume....

Copy link
Contributor

Choose a reason for hiding this comment

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

this is still a ???

We should consider a CI check that we can't merge with that present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can we add a test to exercise this, since we must not have it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnynek I think that you are looking at outdated code. For me there's no longer ??? in the PR.

core/src/main/scala/cats/data/Validated.scala Show resolved Hide resolved
core/src/main/scala/cats/data/Validated.scala Show resolved Hide resolved
final class EitherOpsBinCompat0[A, B](val value: Either[A, B]) extends AnyVal {
/** Returns a [[cats.data.ValidatedNec]] representation of this disjunction with the `Left` value
* as a single element on the `Invalid` side of the [[cats.data.NonEmptyList]]. */
def toValidatedNec[AA >: A]: ValidatedNec[AA, B] = value match {
Copy link
Contributor

Choose a reason for hiding this comment

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

since Validated is covariant, do we need this type parameted?

@codecov-io
Copy link

codecov-io commented Aug 20, 2018

Codecov Report

Merging #2431 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2431      +/-   ##
==========================================
+ Coverage   95.21%   95.22%   +0.01%     
==========================================
  Files         351      351              
  Lines        6369     6387      +18     
  Branches      286      287       +1     
==========================================
+ Hits         6064     6082      +18     
  Misses        305      305
Impacted Files Coverage Δ
core/src/main/scala/cats/data/package.scala 88.88% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/either.scala 99.21% <100%> (+0.04%) ⬆️
core/src/main/scala/cats/syntax/list.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/data/EitherT.scala 97.77% <100%> (+0.03%) ⬆️
core/src/main/scala/cats/syntax/validated.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/data/Validated.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe409cf...46c513d. Read the comment docs.

@LukaJCB
Copy link
Member Author

LukaJCB commented Aug 20, 2018

These are good questions, I just adopted the type signatures from the NonEmptyList counterparts. I'm not sure why those were chosen the way they were chosen in the first place, maybe someone can chime in :)

@LukaJCB LukaJCB force-pushed the give-nec-power branch 2 times, most recently from 8d31a63 to 46c513d Compare August 21, 2018 08:26
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Absent a reason how this is different from NonEmptyList, or how NEL's choices cause serious remorse in 1.0, being consistent with NonEmptyList is compelling to me.

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Thanks @LukaJCB! I agree with Ross's assessment. I won't merge yet in case @johnynek wants to still weigh in.

@LukaJCB LukaJCB merged commit 380f721 into typelevel:master Sep 1, 2018
@LukaJCB LukaJCB deleted the give-nec-power branch September 1, 2018 00:41
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.

6 participants