-
Notifications
You must be signed in to change notification settings - Fork 126
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
Allow to handle "no source root found" in a custom way #525
base: main
Are you sure you want to change the base?
Allow to handle "no source root found" in a custom way #525
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 a lot for the pr @TheElectronWill!
So my largest concern is compat. Right now these are breaking changes to public classes. So I think we might need to rethink this.
Secondly, on the reporting side, we really need to make sure that we complain loudly when we do come across this. If not, people will hit on this and just never report upstream issues when in reality, they are probably issues in the coverage instrumentation.
sourceEncoding: Option[String] | ||
) extends BaseReportWriter(sourceDirectories, outputDir, sourceEncoding) { | ||
sourceEncoding: Option[String], | ||
recoverNoSourceRoot: BaseReportWriter.PathRecoverer |
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.
So I think we'll need to handle this a bit differently. These would be breaking changes for all the reporters, meaning another major bump, and I don't think we want to do another major atm. Can we potentially switch these up to either have a default implementation for this or another way that won't break compat for existing tools that are using this?
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 understand.
I can make the default throw an exception (i.e. keep the old behavior) and allow the "recoverer" to be added up.
.find(root => canonicalSrc.startsWith(root)) | ||
.map(root => canonicalSrc.substring(root.length)) |
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.
Nit, maybe personal preference so take it or leave it
.find(root => canonicalSrc.startsWith(root)) | |
.map(root => canonicalSrc.substring(root.length)) | |
.collectFirst { | |
case root if canonicalSrc.startsWith(root) => | |
canonicalSrc.substring(root.length) | |
} |
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 I hesitated 😄
How can the reporter emit a proper warning without having access to a logger? :c (or maybe it has access to something I've missed). I would like to avoid a basic edit: wait, you mean the "reporting side" is actually "the sbt scoverage plugin" (and mill, gradle, etc.)? I planned to open a PR in it once this one is merged. By making the default throw an exception, and the plugins warn appropriately, it should be ok. |
This introduce a "PathRecoverer" that can take action when a path is not in any source root. In particular, it can replace the path and allow to continue, or skip the element, or fail (and of course emit a warning, or whatever - but I didn't want to make the reporter itself handle logging, they will look better if handled from sbt, mill, etc.)
The next step is to use this in the sbt plugin, and then we can say goodbye to the "no source root found for $x" error!