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

Changing the scoverage report path property key #46

Merged
merged 6 commits into from Apr 8, 2018

Conversation

@BalmungSan
Copy link
Contributor

BalmungSan commented Apr 4, 2018

Fixes #36.

Additionally this removes the misleading default value of the 'sonar.sources' property.

def getSourcesPath(settings: Configuration): String =
settings.get(SourcesPropertyKey).toOption.getOrElse(DefaultSources)
settings.get(SourcesPropertyKey).get

This comment has been minimized.

Copy link
@mwz

mwz Apr 7, 2018

Owner

Could we please not use .get? As per Alexandru's best practices it's a MUST NOT and personally I would also like to avoid using it at all cost.
Today "sonar.sources" might be guaranteed not to be empty, but you never know what happens tomorrow so it's better to be safe than throw an exception.
I'd suggest we keep the default value as it was and we do an additional filter in case if the value is empty:

settings.get(SourcesPropertyKey).toOption
  .filter(_.nonEmpty)
  .getOrElse(DefaultSources)

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 8, 2018

Author Contributor

Uhm, I also don't like to use .get, but the default value is confusing.
Maybe I should add a comment explaining why we use a default value ?

This comment has been minimized.

Copy link
@mwz

mwz Apr 8, 2018

Owner

Yes, it doesn't cause any harm to add a comment.

I'm not sure if I'm looking at the right place, but it looks like sonar-scanner doesn't return an error if you set sonar.sources to an empty string, but I'm not sure if they actually convert that to something else.
Either way, in my opinion, filtering and setting it to a default value is a safe way to go about it.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 8, 2018

Author Contributor

Ok, I'll check at it in a couple of hours.
👍

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 8, 2018

Author Contributor

Hello @mwz , After playing a while with the plugin, I discovered that leaving the sonar.sources property empty during a scan will lead to an empty project (no source files).

So, I will add the default value just to be sure we don't end up throwing an exception in any moment. But, at least for now, this property is completely mandatory.

default
}
}
}

This comment has been minimized.

Copy link
@mwz

mwz Apr 7, 2018

Owner

It might be a bit cleaner if you separate logging from the logic, e.g.:

if (settings.hasKey(ScoverageSensorInternal.OldScoverageReportPathPropertyKey)) {
  logger.warn(
    s"""[scoverage] The property: '${ScoverageSensorInternal.OldScoverageReportPathPropertyKey}' is deprecated,
       | use the new property '${ScoverageSensorInternal.NewScoverageReportPathPropertyKey}' instead.""".stripMargin
  )
} else if (settings.hasKey(ScoverageSensorInternal.NewScoverageReportPathPropertyKey)) {
  logger.info(
    s"""[scoverage] Missing '${ScoverageSensorInternal.NewScoverageReportPathPropertyKey}' property,
       | using the default value: '$default'""".stripMargin
  )
}

settings
  .get(ScoverageSensorInternal.OldScoverageReportPathPropertyKey)
  .toOption
  .getOrElse(
    settings.get(ScoverageSensorInternal.NewScoverageReportPathPropertyKey).toOption.getOrElse(default)
  )

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 8, 2018

Author Contributor

Roger.

@@ -162,7 +178,8 @@ private[scoverage] abstract class ScoverageSensorInternal extends Sensor {

object ScoverageSensorInternal {
private val SensorName = "Scoverage Sensor"
private val ScoverageReportPathPropertyKey = "sonar.scoverage.reportPath"
private val OldScoverageReportPathPropertyKey = "sonar.scoverage.reportPath"
private val NewScoverageReportPathPropertyKey = "sonar.scala.scoverage.reportPath"

This comment has been minimized.

Copy link
@mwz

mwz Apr 7, 2018

Owner

Personally, I'd name the new key just ScoverageReportPathPropertyKey and the deprecated one DeprecatedScoverageReportPathPropertyKey but I'm not too fussy about this.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 8, 2018

Author Contributor

ok

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Apr 8, 2018

That's great, thanks @BalmungSan for making those changes!

@mwz mwz merged commit 940f19f into mwz:master Apr 8, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@BalmungSan BalmungSan deleted the BalmungSan:changing-scoverage-report-key branch Apr 8, 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.