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

Port of #447 for Cats #449

Merged
merged 9 commits into from
Apr 7, 2018
Merged

Port of #447 for Cats #449

merged 9 commits into from
Apr 7, 2018

Conversation

fthomas
Copy link
Owner

@fthomas fthomas commented Mar 4, 2018

This is a port of #447 for Cats. One reason for doing this is to see if the 2.10 build fails in a similar way.

/cc @fommil

@fommil
Copy link
Contributor

fommil commented Mar 4, 2018

Damn.

How do you want to do the 2.11+ restriction? I'm not sure how to do that in your codebase.

@fthomas
Copy link
Owner Author

fthomas commented Mar 5, 2018

The first commit in this PR failed on all Scala versions with:

[error] /home/travis/build/fthomas/refined/modules/cats/shared/src/test/scala/eu/timepit/refined/cats/RefTypeMonadErrorSpec.scala:78:26: diverging implicit expansion for type eu.timepit.refined.api.RefType[F]
[error] starting with method refTypeMonadError in package cats
[error]     val decoder = Decoder[PosInt]
[error]                          ^

Commenting out a seemingly unrelated test in bdc0b26 fixed the 2.11+ builds while the 2.10 build still fails with that error.

@codecov
Copy link

codecov bot commented Mar 5, 2018

Codecov Report

Merging #449 into master will decrease coverage by 1.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   98.01%   96.75%   -1.27%     
==========================================
  Files          57       57              
  Lines         655      677      +22     
  Branches       12       12              
==========================================
+ Hits          642      655      +13     
- Misses         13       22       +9
Impacted Files Coverage Δ
...c/main/scala/eu/timepit/refined/cats/package.scala 100% <100%> (ø) ⬆️
...main/scala/eu/timepit/refined/scalaz/package.scala 100% <0%> (ø) ⬆️

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 2366c6d...96b740d. Read the comment docs.

That is the easiest way to ignore that test with Scala 2.10
@fthomas
Copy link
Owner Author

fthomas commented Mar 5, 2018

@fommil The easiest option to make #447 2.11+ only would be to not run the tests with 2.10. In this PR I choose to run the test for refTypeMonadError only with 2.12. It would be great if the build would support scala-2.11+ source directories but this is not the case right now. So until scala-2.11+ directories are supported or 2.10 support is dropped I'm fine with testing that instance only with 2.12.

@fommil
Copy link
Contributor

fommil commented Mar 5, 2018

Ok great, but the addition of the implicit is breaking one of the existing tests in a way I don't understand.

@fthomas
Copy link
Owner Author

fthomas commented Mar 5, 2018

I'm also okay with not running RefTypeSpecScalazTag with 2.10 and making it 2.12 only.

@fommil
Copy link
Contributor

fommil commented Mar 5, 2018

great! So how do we do that from a practical point of view? I'm not sure how to disable a test.

@fthomas
Copy link
Owner Author

fthomas commented Mar 5, 2018

Moving the test file from refined/modules/scalaz/shared/src/test/scala/eu/timepit/refined/scalaz/ to refined/modules/scalaz/shared/src/test/scala-2.12/eu/timepit/refined/scalaz/ does the trick.

@fthomas fthomas merged commit d11777b into master Apr 7, 2018
@fommil
Copy link
Contributor

fommil commented Apr 7, 2018

Will you cut a release soon with these new things?

@fthomas fthomas deleted the topic/447-cats branch April 9, 2018 17:55
@fthomas
Copy link
Owner Author

fthomas commented Apr 9, 2018

I'll want to process some of the open PRs and need to update the release notes before cutting the next release. That will probably take me one or two weeks.

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