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

Handling multiple and absolute sonar sources #52

Merged
merged 4 commits into from Apr 28, 2018
Merged

Conversation

@mwz
Copy link
Owner

mwz commented Apr 27, 2018

This PR adds support for multiple values set for the sonar.sources setting as well as handles correctly sources being absolute paths (which seems to be the case when using the sonar maven plugin).

Addresses #51.

// remove the extra part of the scala version, e.g 2.11.0 -> 2.11
// TODO: We could use a sem ver parsing here in case someone e.g. skips the patch version.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 27, 2018

Contributor

👍, Absolutely, additionally someone could be using a beta release of scala or something different.

Maybe scaliform already have this implemented?

This comment has been minimized.

Copy link
@mwz

mwz Apr 27, 2018

Author Owner

Yes, quite possibly there is already something in scaliform...
I'll open another issue to look at this properly.

@@ -142,7 +142,7 @@ private[scoverage] abstract class ScoverageSensorInternal extends Sensor {
settings
.get(ScoverageReportPathPropertyKey)
.toOption
.getOrElse(defaultScoverageReportPath)
.getOrElse(defaultScoverageReportPath.toString)

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 27, 2018

Contributor

Well, since you already made the effort of using Paths instead of Strings , why don't we use it for the report path too?

sourcesPrefix <- sourcePrefixes
.filter(cwd.resolve(_).resolve(classNode \@ "filename").toFile.exists)
.take(1)
filename = sourcesPrefix.resolve(classNode \@ "filename")

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 27, 2018

Contributor

I think you could use collectFirst here.

This comment has been minimized.

Copy link
@mwz

mwz Apr 27, 2018

Author Owner

How do you mean?

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 27, 2018

Contributor

I mean something like this:

filename <- sourcesPrefixes.collectFirst {
  case prefix if cwd.resolve(prefix).resolve(classNode \@ "filename").toFile.exist =>
    prefix.resolve(classNode \@ "filename")
}

I can't test it right now, so it may have syntax errors. Also, I think you can assign the file to a variable during the match, so you don't have to call prefix.resolve again.

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented Apr 27, 2018

Awesome work @mwz!
I think that after merging this and making the necessary changes in the README to close #36, you should make a new release.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Apr 27, 2018

Oh yes, sorry I totally forgot about #36.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Apr 27, 2018

Also, I think it wouldn't be a bad idea to release a RC for @coltrey to test and see if this fixes his issues.

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented Apr 27, 2018

Yeah, great idea 👍
Specially to test it in Windows.

mwz added 2 commits Apr 28, 2018
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Apr 28, 2018

Hi @BalmungSan, I've made a few minor tweaks to address your comments and I also added some more tests. If you're happy with those I'll go ahead and merge this PR.

The sample project provided by @coltrey now reports scoverage data, so I'm happy to make a new release once this is merged.

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented Apr 28, 2018

Hi @mwz I've just checked them, everything looks perfect and I'm so happy you add more tests to the scoverage sensor, also thanks for fixing my style issues. I hope my future PRs to have fewer.

PD: Looking forward to see a new release with this fix and the new scoverage report path key.

@mwz mwz merged commit 7173142 into master Apr 28, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@mwz mwz deleted the multiple-and-absolute-sources branch Apr 28, 2018
@mwz mwz added this to Done in sonar-scala Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
sonar-scala
  
Done
2 participants
You can’t perform that action at this time.