-
Notifications
You must be signed in to change notification settings - Fork 69
Update Cats Version and Other Compatible Shifts #204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for sending this.
.travis.yml
Outdated
@@ -14,7 +14,7 @@ jdk: | |||
|
|||
matrix: | |||
include: | |||
- scala: 2.12.0 | |||
- scala: 2.12.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not bump to 2.12.3?
build.sbt
Outdated
lazy val catalystsVersion = "0.0.5" | ||
|
||
lazy val buildSettings = Seq( | ||
organization := "org.typelevel", | ||
scalaVersion := "2.12.1", | ||
crossScalaVersions := Seq("2.10.6", "2.11.8", "2.12.1") | ||
crossScalaVersions := Seq("2.10.6", "2.11.11", "2.12.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change to 2.12.3
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the dilemma is that these require scala-js changes which require changes to your build.sbt
.
I didn't want to do that on the original PR in case you wanted to have a minimal dependency shift, however I can get these done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently so did, 2.11.11
so it needed to be done either way.
Sorry about the sloppy git. |
no worries about the sloppy git. We squash merge anyway. |
👍 lgtm |
I honestly don't know anything about scalajs so this is a bit like shooting in the dark.
Everything is fine, except for a now flagged local val in 2.12, how would you prefer I resolve? Heck, I even just published locally and successfully tested algebird against the updated version with no other additions needed. |
can you remove those two local vals to get the test green? |
I can, however I think it makes the tests that follow irrelevant as I believe they relied on the implicit in scope. Will push that to you though. |
Codecov Report
@@ Coverage Diff @@
## master #204 +/- ##
==========================================
- Coverage 73.84% 73.45% -0.39%
==========================================
Files 37 37
Lines 776 776
Branches 58 58
==========================================
- Hits 573 570 -3
- Misses 203 206 +3
Continue to review full report at Codecov.
|
Okay, so this lowered code coverage, but we weren't actually using the implicits... Huh... I guess we are calling methods regardless, so I can see how that happens, but it is not 100% clear to me if we are actually not using the implicits or if this could be a bug in 2.12.3.... cc @non |
It'd be really nice to get this merged so there is version published for cats 1.0.0-MF. |
I guess we can punt on deep diving on why code coverage dropped here. |
I’ll try to do this today. Sorry for the latency. |
@johnynek Ping! |
Sorry. I started the publish and it took a long time and I forgot and
noticed it timed out on me in another window a few days later.
I’ll try again this weekend.
PS: Colt we need to get you publishing credentials for more stuff. :)
On Fri, Oct 13, 2017 at 09:41 Colt Frederickson ***@***.***> wrote:
@johnynek <https://github.com/johnynek> Ping!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#204 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEJdjqLRs4hw6uNRCwjbzLKW5aub8mcks5sr7zhgaJpZM4PcqfH>
.
--
P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin
|
@johnynek Maybe you could bump to cats 1.0.0-RC1 and skip 1.0.0-MF? 🤣 |
seems like a good plan! |
so, it's pretty broken currently due to discipline upgrade. looking. |
this PR is what we need to accommodate. PS: this is the pain of making cosmetic changes to fix warts.... ugh... I don't have the time to deal with this now. Is anyone interested in sending a PR to fix this? |
PS: now that I'm cranky about this again... another reason to live with warts is because fixing breakage costs us labor we often don't have. |
@johnynek what should we do? Align algebra to the new cats-kernel style, or import from the old cats-kernel-laws the pieces in the old style, so that we don't propagate the breakage downstream? On the Spire side, I'd like to make quite a bit of changes before we stabilize on 1.0, but how far we go will depend on other maintainers. Could be that we follow the new cats-kernel style, or that we find a third way (especially as we'd like to test primitive types such as |
I'm putting together a PR to update to the new law encoding. I'd appreciate if someone could answer a couple of questions on gitter :) |
Importing the old style may be the easiest fix. I don’t know what @LukaJCB had in mind. Honestly I was never crazy about this super inheritance driven approach but it is what we have I guess. |
Hey guys, Sorry to bump this thread but we have projects that are heavily using
|
I noticed after writing the message that this PR is already merged with cats 1.0-MF. Do we know when next release will be cut? |
2.12 updates requires and update to scalajs and I didn't know if you wanted to make those shifts as well, so I figured I would start off small.