Add support for versions with less segments (#212) #214
Conversation
|
|
||
| log.info(s"$projectName: found ${backErrors.size+forwErrors.size} potential binary incompatibilities while checking against $module $filteredNote") | ||
| ((backErrors map {p: core.Problem => prettyPrint(p, "current")}) ++ | ||
| (forwErrors map {p: core.Problem => prettyPrint(p, "other")})) foreach { log.info } |
There was a problem hiding this comment.
I think we should preserve the logic that bumps this to up to warn/error level, not just info level.
| val (epoch, major, minor) = version match { | ||
| case ModuleVersion(e, m, mi) => (e, m, mi) | ||
| case ModuleVersion(e, m, null) => (e, m, "0") | ||
| case ModuleVersion(e, null, null) => (e, "0", "0") |
There was a problem hiding this comment.
I don't think these aren't going to ever match. The first one will always match:
Welcome to Scala 2.12.6 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_144).
Type in expressions for evaluation. Or try :help.
scala> val ModuleVersion = """(\d+)\.?(\d+)?\.?(.*)?""".r
ModuleVersion: scala.util.matching.Regex = (\d+)\.?(\d+)?\.?(.*)?
scala> def check(version: String) = {
| val (epoch, major, minor) = version match {
| case ModuleVersion(e, m, mi) => (e, m, mi)
| case ModuleVersion(e, m, null) => (e, m, "0")
| case ModuleVersion(e, null, null) => (e, "0", "0")
| }
| (epoch, major, minor)
| }
check: (version: String)(String, String, String)
scala> check("1.2.3")
res0: (String, String, String) = (1,2,3)
scala> check("1.2")
res1: (String, String, String) = (1,2,"")There was a problem hiding this comment.
Yea, your totally right. I got that wrong.
There was a problem hiding this comment.
Thanks for looking at this @2m!
I have a couple of concerns. One about what looks to me like a regression in logging behaviour that seems accidental. The second is around the version matching code logic.
But other than that, this looks good to go to me!
|
Thanks for a quick review. Pushed more commits that address the feedback. |
| (forwErrors map {p: core.Problem => prettyPrint(p, "other")})) foreach { log.error } | ||
| (forwErrors map {p: core.Problem => prettyPrint(p, "other")})) foreach { p => | ||
| if (failOnProblem) log.error(p) | ||
| else log.info(p) |
There was a problem hiding this comment.
Let's keep warning in this case, IMO.
|
Thanks @2m! |
Fixes #212