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

Add mima for kernel #1159

Merged
merged 1 commit into from
Jun 26, 2016
Merged

Add mima for kernel #1159

merged 1 commit into from
Jun 26, 2016

Conversation

johnynek
Copy link
Contributor

This adds mima for kernel, but it does not pass, so it is not added to travis yet.

As discussed when moving kernel from algebra, the thinking was we could be more conservative there. I would like to turn this on after the next release (I guess 0.7.0) and I hope we can be conservative with kernel updates so that algebra is not too tightly coupled to cats.

@johnynek
Copy link
Contributor Author

relates to #814 (does not close, since we have failing test case now).

@codecov-io
Copy link

Current coverage is 88.80%

Merging #1159 into master will not change coverage

@@             master      #1159   diff @@
==========================================
  Files           232        232          
  Lines          3073       3073          
  Methods        3021       3021          
  Messages          0          0          
  Branches         49         49          
==========================================
  Hits           2729       2729          
  Misses          344        344          
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by c4f5d1c...3af6587

@kailuowang
Copy link
Contributor

We could also make the previousArtifacts dynamically check previous bug fix version, like

  lazy val mimaSettings = Seq(
    mimaPreviousArtifacts := (if (scalaBinaryVersion.value != "2.10") {
      Version(version.value).flatMap {
        // we are currently only checking binary compat with the previous bugfix version
        case Version(major, Some(minor), Some(bugfix), _) if bugfix > 0 =>
          Some(Set(organization.value %% moduleName.value % Seq(major, minor, bugfix - 1).mkString(".")))
        case  _ => None
      }
    } else None).getOrElse(Set.empty)
  )

@benhutchison
Copy link
Member

In my workplace, many Scalaz/Argonaut-based projects have become binary- and source- compatibility minefields. Pleased to see cats taking version compatibility seriously from the outset; I hope concern from compatibility becomes part of Cats' "cultural DNA".

@johnynek
Copy link
Contributor Author

big +1 to this. binary compatibility should be taken very seriously for a library like cats that hopes to be at the bottom of a lot of dependencies.

@johnynek
Copy link
Contributor Author

@kailuowang #1159 (comment) that is a great idea. In fact, I think we can do something like this to verify semver:

if major == 0 then we check previous bugfix, if major > 0 we check previous bugfix and previous patch at version 0 perhaps?

That said, my overall point is that I think cats-kernel should enforce binary compatibility sooner (maybe like now), so that algebra can move close to a 1.0 release.

@kailuowang
Copy link
Contributor

Sorry not sure I follow completely. Are you suggesting that for kernel we maintain binary comp between minor versions between 0.7.0 and 1.0.0 ?

@johnynek
Copy link
Contributor Author

Yes, I am. That was my understanding when we discussed moving that code
from algebra to cats (since it has changed very little in two years, this
does not seem onerous).

On Saturday, June 25, 2016, Kai(luo) Wang notifications@github.com wrote:

Sorry not sure I follow completely. Are you suggesting that for kernel we
maintain binary comp between minor versions between 0.7.0 and 1.0.0 ?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1159 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAEJdmQq7UieVdS2tgCSTeUlCo89hNuvks5qPdpLgaJpZM4I-OQp
.

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

@ceedubs
Copy link
Contributor

ceedubs commented Jun 26, 2016

👍 thanks @johnynek. I'll create a follow-up issue to enable this after the next release.

@ceedubs ceedubs merged commit 89bc41f into typelevel:master Jun 26, 2016
@ceedubs ceedubs mentioned this pull request Jun 26, 2016
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