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.

Sign up for GitHub

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

Jump to bottom

minor: Fix sonarcube warnings #736

Merged
merged 2 commits into from
Aug 21, 2023
Merged

minor: Fix sonarcube warnings #736

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e437918
minor: Fix sonarcube warnings
wajda Aug 16, 2023
637b98e
LineageWalker refactoring
wajda Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .sonarcloud.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
sonar.cpd.exclusions=/core/src/test/**/*.scala,integration-tests/src/test/**/*.scala
sonar.cpd.exclusions=**/*Spec.*,**/*Test.*,**/*Tests.*
5 changes: 4 additions & 1 deletion core/src/test/scala/za/co/absa/spline/harvester/conf/HadoopConfigurationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,8 @@ object HadoopConfigurationTest {
Configuration.addDefaultResource(
s"${this.getClass.getPackage.getName.replaceAllLiterally(".", "/")}/hdfs-test-conf.xml")

def init(): Unit = {}
def init(): Unit = {
// A singleton activator.
// Called from the class instances to ensure execution of the companion object's constructor code.
}
}
10 changes: 5 additions & 5 deletions ...la/za/co/absa/spline/harvester/postprocessing/DataSourcePasswordReplacingFilterSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ class DataSourcePasswordReplacingFilterSpec extends AnyFlatSpec with Matchers wi
"b" -> Seq(null),
"c" -> None,
"m" -> Map(
"url" -> "jdbc:postgresql://bob:secret@someHost:somePort/someDB",
"url" -> "jdbc:postgresql://bob:secret@someHost:somePort/someDB", // NOSONAR
"dbtable" -> "someTable",
"user" -> "someUser",
"PASSPHRASE" -> "somePassword" // NOSONAR
),
"url" -> "jdbc:postgresql://bob:secret@someHost:somePort/someDB",
"url" -> "jdbc:postgresql://bob:secret@someHost:somePort/someDB", // NOSONAR
"dbtable" -> "someTable",
"user" -> "someUser",
"password" -> "somePassword" // NOSONAR
Expand All @@ -92,14 +92,14 @@ class DataSourcePasswordReplacingFilterSpec extends AnyFlatSpec with Matchers wi
"a" -> 42,
"b" -> Seq(null),
"c" -> None,
"url" -> "jdbc:postgresql://bob:*****@someHost:somePort/someDB",
"url" -> "jdbc:postgresql://bob:*****@someHost:somePort/someDB", // NOSONAR
"m" -> Map(
"url" -> "jdbc:postgresql://bob:*****@someHost:somePort/someDB",
"url" -> "jdbc:postgresql://bob:*****@someHost:somePort/someDB", // NOSONAR
"dbtable" -> "someTable",
"user" -> "someUser",
"PASSPHRASE" -> "*****" // NOSONAR
),
"url" -> "jdbc:postgresql://bob:*****@someHost:somePort/someDB",
"url" -> "jdbc:postgresql://bob:*****@someHost:somePort/someDB", // NOSONAR
"dbtable" -> "someTable",
"user" -> "someUser",
"password" -> "*****" // NOSONAR
Expand Down
2 changes: 1 addition & 1 deletion examples/src/main/scala/za/co/absa/spline/SparkApp.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,5 @@ abstract class SparkApp
*/
val spark: SparkSession = sparkBuilder.getOrCreate()

protected override def _sqlContext: SQLContext = spark.sqlContext
protected override def _sqlContext: SQLContext = spark.sqlContext // NOSONAR
}
9 changes: 2 additions & 7 deletions integration-tests/src/main/scala/za/co/absa/spline/test/LineageWalker.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,9 @@ class LineageWalker(

private def dependsOnRec(ref: AttrOrExprRef, id: String): Boolean = ref match {
case AttrRef(attrIfd) =>
if (attrIfd == id) true
else dependsOnRec(attrMap(attrIfd).childRefs, id)
attrIfd == id || dependsOnRec(attrMap(attrIfd).childRefs, id)
case ExprRef(exprId) =>
if (exprId == id) true
else {
if (litMap.contains("exprId")) false
else dependsOnRec(funMap(exprId).childRefs, id)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's shorter, but for me, the new version is harder to understand.

Copy link
Contributor Author

@wajda wajda Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What about introducing an intermediate val? For example?

lazy val attrChildIds = attrMap(attrId).childRefs
lazy val anyChildRefsDepends = dependsOnRec(attrChildRefs, id)
attrIfd == id || anyChildRefsDepends

The main motivation was to comply with the Sonar rule.

Subjectively, I understand why this is usually recommended approach. First, it's shorter, but also it is more plural for the reader. There is less nesting and branching involved for the reader. You have entire resulted condition written in one sentence that IMO looks more natural, e.g. condition = this or that and not something_else. But I agree that a lot depends on naming. So I wouldn't hesitate choosing better names for functions, or introducing intermediate variables.

On a side note, I just noticed that attrIfd is probably a typo. Shouldn't it be attrId? Also dependsOnRef instead of dependsOnRec?

Copy link
Contributor

Choose a reason for hiding this comment

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

For simple cases, It may be ok, but here the second condition is more complex. The nesting and branching is still there, it just now must happen inside the reader's head instead of in the code. The reader must consider short-circuiting, negate one expression and operator precedence, and then build a truth table to make sense of it. The Sonar just doesn't consider any of that.

Shorter is not better, I think we should aim to minimize cognitive load.

Why do you mean by plural?

attrIfd yes typo
dependsOnRec no it means recursive - just private name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plural -> fluent. Crazy typo :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me refactor it a bit and I'll propose another variant.

exprId == id || !litMap.contains("exprId") && dependsOnRec(funMap(exprId).childRefs, id)
}

}
Expand Down
3 changes: 0 additions & 3 deletions integration-tests/src/main/scala/za/co/absa/spline/test/fixture/spline/SplineFixture.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

package za.co.absa.spline.test.fixture.spline

import org.apache.commons.configuration.BaseConfiguration
import org.apache.spark.sql.SparkSession


Expand All @@ -26,8 +25,6 @@ trait SplineFixture {
}

object SplineFixture {
def EmptyConf = new BaseConfiguration

def extractTableIdentifier(params: Map[String, Any]): Map[String, Any] =
params
.apply("table").asInstanceOf[Map[String, _]]
Expand Down
2 changes: 0 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@
<arg>-deprecation</arg>
<arg>-unchecked</arg>
<arg>-Ywarn-numeric-widen</arg>
<!--<arg>-Ywarn-dead-code</arg>-->
<!--<arg>-Ywarn-value-discard</arg>-->
</args>
</configuration>
<executions>
Expand Down
Loading