-
Notifications
You must be signed in to change notification settings - Fork 10
Invert version defaulting #7
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
Invert version defaulting #7
Conversation
@@ -3,9 +3,26 @@ package sbttravisci | |||
import sbt._, Keys._ | |||
|
|||
object TravisCiPlugin extends AutoPlugin { | |||
private val DefaultScalaVersion = "2.10.6" // for compatibility with stock sbt |
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.
Should we default to 2.12.0 instead? I just set it to 2.10 since that's the default SBT behavior if you don't specify scalaVersion
.
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.
I'm still afk so can't explain fully, but I think we should bomb by default. Avoids circular dependencies between the two keys that I had to avoid in my wartremover patch.
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.
I'm ok with that. Will implement.
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.
No .travis.yml with scala versions, no party.
…'t parse .travis.yml
override def requires = plugins.JvmPlugin | ||
override def trigger = allRequirements | ||
override def buildSettings = Seq( | ||
isTravisBuild := sys.env.get("TRAVIS").isDefined, |
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.
Env vars are global, so it would be even better to set this in globalSettings. Won't change anything for 99% of the cases, but it's more correct.l technically. Its in the docs: new key defaults should be set in globalSettings.
Also this is a great addition.
@@ -24,7 +24,9 @@ Other than that, as `sbt-travisci` is an AutoPlugin that is all that is required | |||
|
|||
## Detail | |||
|
|||
`crossScalaVersions in ThisBuild` will be automatically set to the scala versions in `.travis.yml`. | |||
`crossScalaVersions in ThisBuild` will be automatically set to the scala versions in `.travis.yml`. `scalaVersion in ThisBuild` will be automatically set to the `last` version in `crossScalaVersions`, and so by default, SBT will assume you want to develop under the *last* version listed in `.travis.yml`. |
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.
s/SBT/sbt/ please :-) even at the start of the sentence. Think eBay or iPhone
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.
I wasn't aware of the branding change. Is this just part of discouraging "sbt's" use as an acronym?
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.
yep. Not just to discourage "simple build tool" but also "scala build tool" as it works as a java build tool too
This is great. Let me get back to a keyboard and re-read and respond to everything (and it wasn't too much spam, its great stuff). Thanks. |
Again, this is great. And I agree to all parts of it. Some Scala builds don't depend on TRAVIS_SCALA_VERSION, but I'm happy for this to be opinionated as such. |
if (isTravisBuild.value) | ||
sys.env.get("TRAVIS_SCALA_VERSION").get | ||
else | ||
crossScalaVersions.value.last // sort .travis.yml versions in ascending order |
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.
Turns out that:
The jobs which are explicitly included inherit the first value of the expansion keys defined.
This includes (IIUC) jobs added with matrix.include
, job.include
or build stages.
So... in hindsight, the default should be .head
not .last
... Not sure what to do.
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.
Ahhhh, interesting! Good point. I think it's worth changing, but obviously it will require some sort of version bump. Probably 2.0?
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.
Yeah. I don't have an immediate need, but we can go that way if it comes.
See #6 for motivation. Also adds an
isTravisBuild
setting, which is generally useful in more complex builds.