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

ScalaPB additional args #1002

Merged
merged 7 commits into from
Nov 27, 2020
Merged

Conversation

DanielBlanco
Copy link
Contributor

@DanielBlanco DanielBlanco commented Nov 13, 2020

@lefou can you please review, and let me know if I'm missing something?

The plan with this change is to allow additional args for scalaPBC, this means I can just do this in my code for zio-grpc to work:

object example extends ScalaPBModule {
  def scalaVersion = "2.12.6"
  def scalaPBVersion = "0.7.4"

   override def scalaPBAdditionalArgs = Seq(
        s"--zio_out=${T.dest.toIO.getCanonicalPath}"
   )
}

Also, refactored the code to allow retrieving the compilation args,
this will ease testing.
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your pull request looks good to me. Personally, I'd name the new target "additionalArgs" instead of "customArgs" as that better transports the fact that other args logic will still be preserved and it also matches what we have in other plugins, but that's only a suggestion and up to you.

But I'd like to have some more ScalaDoc, at least on the newly added and changed targets. These will help any user of the plugin who edits their buildfile in an IDE.

contrib/scalapblib/src/ScalaPBWorker.scala Outdated Show resolved Hide resolved
@DanielBlanco DanielBlanco requested a review from lefou November 18, 2020 04:33
@DanielBlanco DanielBlanco changed the title ScalaPB custom args ScalaPB additional args Nov 18, 2020
@DanielBlanco DanielBlanco requested a review from lefou November 27, 2020 15:43
@lefou lefou merged commit ff654fc into com-lihaoyi:master Nov 27, 2020
@lefou
Copy link
Member

lefou commented Nov 27, 2020

Thank you, @DanielBlanco !

@lefou lefou added this to the after 0.9.3 milestone Nov 27, 2020
@DanielBlanco DanielBlanco deleted the scalapb-custom-args branch November 27, 2020 17:09
@chikei
Copy link
Contributor

chikei commented Dec 3, 2020

Actually this PR might break ScalaPB module as ScalaPB module append generatedSources with compileScalaPB which was the scalapbc output path, but here compilationArgsScalaPB incorrectly write output path using T.dest from compilationArgsScalaPB. @DanielBlanco

You can test these by uncomment test calledDirectly.

@DanielBlanco
Copy link
Contributor Author

OK, I'll try the test and come up with a solution if I can. Thx @chikei.

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.

3 participants