-
Notifications
You must be signed in to change notification settings - Fork 54
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
Fix addition of unwanted options to console task #66
Fix addition of unwanted options to console task #66
Conversation
@DavidGregory084 Addition of the new key and vals broke the binary compatibility with |
6b42ab1
to
1a9f26e
Compare
… of unwanted options to console task Fix test reference Fix test reference Fix test reference
e0aa717
to
20f29c4
Compare
@seveneves I don't mind the addition of new methods (I don't think we should promise forward binary compatibility in this plugin), but I think the main problem is this: [error] * method tpolecatConsoleOptionsFilter()scala.Function1 in object io.github.davidgregory084.TpolecatPlugin#autoImport does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("io.github.davidgregory084.TpolecatPlugin#autoImport.tpolecatConsoleOptionsFilter") Do you think there is a way that we can make this change and still retain that method? If not I don't mind cutting 0.3.0. |
Yes of course, it was removed thus it is backwards incompatible. I didn't consider it to be public API till I noticed it's mentioned in the readme. I think it is possible to change But it won't be possible to still keep the following working
And I think it is a nicer sbt "API" just to have a But this is just my opinion and you might think differently. Let me know if I should do it via |
@DavidGregory084 ☝️ |
val filters = scalacOptionsFor(scalaV, tpolecatExcludeOptions.value).toSet | ||
val newOptions = scalacOptionsFor(scalaV, tpolecatScalacOptions.value) | ||
(previous ++ newOptions).filterNot(filters).distinct | ||
} | ||
), | ||
tpolecatDevModeOptions := ScalacOptions.default, |
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.
@DavidGregory084 I would also move this to build or global settings as "common" defaults. If one uses ThisBuild / tpolecatDevModeOptions
it will have same problems as did scalacOptions
in #60. Wdyt?
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 we can definitely try that!
Yes I totally agree - when the |
@@ -129,7 +123,13 @@ object TpolecatPlugin extends AutoPlugin { | |||
|
|||
override def projectSettings: Seq[Setting[_]] = Seq( | |||
Def.derive( | |||
scalacOptions ++= scalacOptionsFor(scalaVersion.value, tpolecatScalacOptions.value) | |||
scalacOptions := { |
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 have just noticed the change from ++=
back to :=
- does that still behave correctly in relation to the issue you saw on #60?
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.
Yes. Look at the back reference of scalacOptions.value
in the body of the task
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.
Cool, just wanted to understand whether it affected the fix of the previous issue 😛
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 added a test to catch the problem. It should be working :)
Other than the question about In order to unblock your build I think we need to set We can also empty out the |
I set both of them to empty values. I'm not entirely sure how to populate |
That's absolutely fine - since we are going to move to a new reference "mimaPreviousArtifact" there will be no binary compatibility issues to filter out, hopefully that makes sense. The existing filters were there to ignore methods that were added since 0.2.0. |
Thanks for this @seveneves! |
#61 broke filtering of console
scalacOptions
This is fixed with the help of a new setting key
tpolecatExcludeOptions
that is used to filter out all options and not just the current additions.Also, this PR adds tests to
console
,ThisProject
andThisBuild
direct usages ofscalacOptions