-
Notifications
You must be signed in to change notification settings - Fork 89
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
Fixes ScalaCaseClassRenderer for #110 #111
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 the PR, @damdev! I have a couple of finicky questions about the fix, but by-and-large it looks good!
@@ -17,6 +17,7 @@ case class ScalaCaseClassRenderer(options: Seq[BuildInfoOption], pkg: String, ob | |||
s"package $pkg", | |||
"", | |||
"import scala.Predef._", | |||
"import scala.Any", |
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 you explain why this is necessary?
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.
When running the scripted test I was having this error /tmp/sbt_4647108f/caseclassrenderer/target/scala-2.11/src_managed/main/sbt-buildinfo/BuildInfo.scala:8: not found: type Any
Searching into this I found this may be related bug sbt/sbt#2572
@@ -5,7 +5,7 @@ case class ScalaCaseClassRenderer(options: Seq[BuildInfoOption], pkg: String, ob | |||
override def extension = "scala" | |||
|
|||
val traitNames = options.collect{case BuildInfoOption.Traits(ts @ _*) => ts}.flatten | |||
val objTraits = if (traitNames.isEmpty) "" else " extends " ++ traitNames.mkString(" with ") | |||
val objTraits = if (traitNames.isEmpty) "" else "extends " ++ traitNames.mkString(" with ") |
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 think we should keep this as it, to avoid trailing spaces when there are no traits to mix in.
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.
Ah I understand this change now. (How did I misunderstand??)
private def caseClassEnd = List("}") | ||
|
||
private def caseObjectLine(buildInfoResults: Seq[BuildInfoResult]) = List( | ||
s"case object $obj {", | ||
s" def apply(): $obj = new $obj(${buildInfoResults.map(_.value).map(quote).mkString(",")})", | ||
s" def apply(): $obj = new $obj(${buildInfoResults.map(r => s"\n ${r.identifier} = ${quote(r.value)}").mkString(",")})", |
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.
Could you explain why you want to switch to using named parameters 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.
I just see the generated code more clear than a whole line with the values.
Hi, what do you think about this PR? @dwijnand |
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.
Sure! LGTM
@@ -5,7 +5,7 @@ case class ScalaCaseClassRenderer(options: Seq[BuildInfoOption], pkg: String, ob | |||
override def extension = "scala" | |||
|
|||
val traitNames = options.collect{case BuildInfoOption.Traits(ts @ _*) => ts}.flatten | |||
val objTraits = if (traitNames.isEmpty) "" else " extends " ++ traitNames.mkString(" with ") | |||
val objTraits = if (traitNames.isEmpty) "" else "extends " ++ traitNames.mkString(" with ") |
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.
Ah I understand this change now. (How did I misunderstand??)
I won't have time to publish the plugin for a while, though. Sorry. |
Fixes #110