Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added support for scalac-scoverage-plugin > 2.0.0 #107
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
Added support for scalac-scoverage-plugin > 2.0.0 #107
Changes from all commits
213462f
ea48709
63e0174
034e40c
6ec7b6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 this may have to be pinned to e.g. 2.13.10, to actually work? (going by available maven coordinates)
You may also want to bump the plugin version to 2.0.8 already
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.
It looks like this is only being used by the reporter down below, which doesn't need to be the exact version of Scala, only the binary 2.12, 2.13, or 3. The actual compiler plugin is what needs to be the full version.
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.
referencing that in the variable name would make it unmistakable.
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.
Switched to targeting Java 1.8
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 guess something like this would make the failure more evident?
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.
The current behaviour will be to fail to resolve the scoverage compiler plugin for Scala 3 as no plugins are currently published newer than 2.13.10
(Link to MVNRepository here)
This will cause a build failure. It's not as nice as explicitly notifying the user.
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.
The artifact version in the test you referenced is the version of the scoverage plugin. The scala version is calculated through
resolveScalaVersion()
.That function will default to an explicitly configured scala version or a search for the scala standard library. If I understand correctly, the Scala 3 standard library uses different coordinates from the earlier versions
org.scala-lang:scala3-library
.If the user is depending on the scala 3 standard library
resolveScalaVersion()
will fail to detect the scala version resulting in a warning being emitted and the execution of the mojo being skipped.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.
To me that sounds like acceptable behavior - @ckipp01 since an explicit scala-3-warning would need explicit scala-3 detection, I would leave that to a separate fix?
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 that would be fine, but I'd for sure want to try to get that in before we attempt to do another release.