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.

Sign up for GitHub

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

Jump to bottom

add unit test for variance on methods in EitherT #1629

Merged
merged 3 commits into from
May 15, 2017
Merged

add unit test for variance on methods in EitherT #1629

merged 3 commits into from
May 15, 2017

Conversation

jtjeferreira
Copy link
Contributor

related to #1627 and #1506

kailuowang
kailuowang reviewed Apr 24, 2017
View reviewed changes
tests/src/test/scala/cats/tests/EitherTTests.scala Outdated

for {
s1 <- EitherT(either1)
s2 <- EitherT.right[Id, AppError, String]("1".pure[Id])
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to write EitherT.pure[Id, AppError]("1") here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ceedubs
Copy link
Contributor

ceedubs commented Apr 25, 2017

@jtjeferreira thanks!

It looks like there's a compile error in the test:

[error] /home/travis/build/typelevel/cats/tests/src/test/scala/cats/tests/EitherTTests.scala:398: wrong number of type parameters for method pure of type [F[_], A]=> cats.data.EitherT.PurePartiallyApplied[F,A]
[error]       s2 <- EitherT.pure[Id, AppError, String]("1")

@jtjeferreira
Copy link
Contributor Author

@ceedubs that's very weird because I cant reproduce it locally...

@kailuowang
Copy link
Contributor

kailuowang commented Apr 25, 2017 •
edited
Loading

@jtjeferreira your local branch is probably outdated.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 25, 2017

@jtjeferreira here's how I'm able to reproduce it locally:

❯ sbt
...
> coreJVM/doc
[info] Main Scala API documentation to /Users/cody/code/cats/core/.jvm/target/scala-2.12/api...
model contains 525 documentable templates
[warn] /Users/cody/code/cats/core/src/main/scala/cats/data/Validated.scala:248: Variable s undefined in comment for method ensureWith in class Validated
[warn]     * scala> Validated.valid("ab").ensureWith(s => new IllegalArgumentException(s"Must be longer than 3, provided '$s'"))(_.length > 3)
[warn]                                                                                                                     ^
[warn] one warning found
[info] Main Scala API documentation successful.
[success] Total time: 46 s, completed Apr 25, 2017 12:45:43 PM

I think that we have the fatal warnings flag enabled, so this ends up being a fatal error. Note that it doesn't complain if you use ++2.10.6, I think because we've disabled scaladoc creation for Scala 2.10 due to some other issues.

@jtjeferreira
Copy link
Contributor Author

my local branch was outdated, rebased and fixed. waiting for Travis...

@codecov-io
Copy link

codecov-io commented Apr 25, 2017 •
edited
Loading

Codecov Report

Merging #1629 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1629   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files         240      240           
  Lines        3937     3937           
  Branches      139      138    -1     
=======================================
  Hits         3676     3676           
  Misses        261      261

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 97a3d26...2e6d78a. Read the comment docs.

ceedubs
ceedubs approved these changes Apr 25, 2017
View reviewed changes
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 @jtjeferreira!

kailuowang
kailuowang reviewed Apr 28, 2017
View reviewed changes
tests/src/test/scala/cats/tests/EitherTTests.scala
for {
s1 <- EitherT(either1)
s2 <- EitherT.pure[Id, AppError]("1")
} yield s1 ++ s2
Copy link
Contributor

Choose a reason for hiding this comment

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

I might've missed something but it's not very clear to me why there are all 4 tests needed here. The types of the two EitherTs in the first, second and last test are exactly the same, aren't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kailuowang hi sorry for the late reply. These tests are just to prevent regressions like the one I was going to introduce in #1627 so for the sake of completion I covered the different use cases...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on the other hand this was more problematic issue before #1583 was merged...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @jtjeferreira I think that there is some value in testing the inference here, and I certainly don't think the extra unit tests hurt anything. 👍 from me.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I guess I was just nitpicking.

@kailuowang kailuowang merged commit cb949b8 into typelevel:master May 15, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-MF May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ceedubs ceedubs ceedubs approved these changes

@kailuowang kailuowang kailuowang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0-MF
Development

Successfully merging this pull request may close these issues.
4 participants
@jtjeferreira @ceedubs @kailuowang @codecov-io