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

testkit: extract targetroot from scalacOptions #1406

Merged
merged 2 commits into from
May 17, 2021

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented May 16, 2021

Second take at #1275 leveraging #1389 (with the addition of the first preliminary commit, allowing the targetroot to be repeated in the CLI when dealing with a complex classpath)

Unlocks scalacenter/sbt-scalafix#219

@bjaglin bjaglin force-pushed the testkit-scala3 branch 2 times, most recently from a6bc35c to 3c57602 Compare May 16, 2021 20:19
@bjaglin bjaglin changed the title testkit: add semanticdbTargetRoot to classpath automatically testkit: add semanticdbTargetRoot to classpath based on scalacOptions May 16, 2021
@bjaglin bjaglin changed the title testkit: add semanticdbTargetRoot to classpath based on scalacOptions testkit: add targeroot to classpath based on scalacOptions May 16, 2021
@bjaglin bjaglin force-pushed the testkit-scala3 branch 3 times, most recently from 0835289 to 20b3be8 Compare May 16, 2021 21:57
@bjaglin bjaglin changed the title testkit: add targeroot to classpath based on scalacOptions testkit: extract targeroot from scalacOptions May 16, 2021
@bjaglin bjaglin changed the title testkit: extract targeroot from scalacOptions testkit: extract targetroot from scalacOptions May 16, 2021
@@ -34,7 +34,7 @@ object Mima {
ProblemFilters.exclude[Problem]("scalafix.testkit.SemanticRuleSuite.*"),
ProblemFilters.exclude[MissingClassProblem]("scalafix.testkit.SyntacticRuleSuite$"),
ProblemFilters.exclude[Problem]("scalafix.testkit.SyntacticRuleSuite"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("scalafix.interfaces.ScalafixArguments.withSemanticdbTargetroot")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unreleased

@bjaglin bjaglin marked this pull request as ready for review May 16, 2021 22:08
@bjaglin bjaglin requested a review from mlachkar May 16, 2021 22:08
* client should not have to manually declare the semanticdb directories
  through the inputClasspath
* rely on args.validatedClasspath for proper Scala 3 support
Copy link
Collaborator

@mlachkar mlachkar left a comment

Choose a reason for hiding this comment

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

I didn't think about having multiple targetroot directories.
I do prefer this approach for specifying semanticdbTargetRoot.

@bjaglin
Copy link
Collaborator Author

bjaglin commented May 17, 2021

I didn't think about having multiple targetroot directories.

Me neither at first, and I didn't expect this PR to take me there (since it's about inferring it from scalacOptions in testkit, meaning that it can't be repeated) but I think it demonstrates how/why having a separate CLI argument makes sense: specifically when the input is so complex that it was compiled in several scalac invokations. Of course, in that case we would like to provide several "sets" of scalacOptions, but that's another story.

We might want to expose semanticdbTargetroots as a property in testkit to handle such use-cases, but I thought we could live without it for now and revisit later. I think it's fair for the testkit to follow what the CLI has been doing until now anyway.

@bjaglin bjaglin merged commit f41dec8 into scalacenter:main May 17, 2021
@mlachkar
Copy link
Collaborator

Ok, let's do that like this, but I think if we start using withTargetroots in the sbt-plugin, we should do the same in the testkit, even if we retrieve the targetroot from the scalacOptions for now.

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