Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

Changing the scoverage report path property key #46

Merged
merged 6 commits into from
Apr 8, 2018

Conversation

BalmungSan
Copy link
Contributor

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
  )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@mwz
Copy link
Member

mwz commented Apr 8, 2018

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

@mwz mwz merged commit 940f19f into sonar-scala:master Apr 8, 2018
@BalmungSan BalmungSan deleted the changing-scoverage-report-key branch April 8, 2018 23:27
@mwz mwz added this to Done in sonar-scala Jul 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
sonar-scala
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants