You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "olabusayoT (via GitHub)" <gi...@apache.org> on 2025/02/06 17:24:45 UTC
[PR] Fix deprecated auto application of () to method invocations [daffodil]
olabusayoT opened a new pull request, #1422:
URL: https://github.com/apache/daffodil/pull/1422
- add support for 2.13 scala options
- add explicit () to some method invocations
- fix some collections to explicitly used toSeq to return an immutable Seq
- replace string concat with + to interpolated strings
DAFFODIL-2152
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT merged PR #1422:
URL: https://github.com/apache/daffodil/pull/1422
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945372115
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -352,12 +352,12 @@ class TunableEnumDefinition(
""".trim.stripMargin
private val scalaEnums = {
- val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
+ val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" }
Review Comment:
But to your point sticking with string concatenation will look something like
e/head.toUpper.toString + e.tail
Is that preferable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945356231
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -57,7 +57,7 @@ object XercesValidatorFactory {
if (config.hasPath(XercesValidator.name))
config.getStringList(XercesValidator.name).asScala
else Seq.empty
- XercesValidator.fromFiles(schemaFiles)
+ XercesValidator.fromFiles(schemaFiles.toSeq)
Review Comment:
It turns out Scala 2.13 doesn't always complain about all nullary methods
"For reasons of backwards compatibility, Scala 3 for the moment also auto-inserts () for nullary methods that are defined in Scala 2, or that override a method defined in Scala 2."
Which toSeq seems to fall under, shall I add it to toSeq?
https://docs.scala-lang.org/scala3/reference/dropped-features/auto-apply.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945360050
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -352,12 +352,12 @@ class TunableEnumDefinition(
""".trim.stripMargin
private val scalaEnums = {
- val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
+ val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" }
Review Comment:
Char + is conctatenation to a string is deprecated in 2.13, and string interpolation was the recommended fix from Intellij, we can either do that or convert the char to a string before using +. Though I prefer string interpolation personally
https://docs.scala-lang.org/scala3/guides/migration/incompat-dropped-features.html#any2stringadd-conversion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945371865
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -352,12 +352,12 @@ class TunableEnumDefinition(
""".trim.stripMargin
private val scalaEnums = {
- val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
+ val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" }
Review Comment:
Gaaak. A String is a `Seq[Char]`. Appending `T` to `Seq[T]` at the front of the sequence is one of the most basic operations in functional programming. It is `cons` from Lisp.
Perhaps `+` is the wrong operator here? Maybe these should be the actual `cons` operator i.e., is that `char :: string` or `char +: string` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945480188
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -292,7 +292,7 @@ class EnumListTunable(
"Nil"
} else {
val defaultSeq =
- trimmedDefault.split("\\s+").map(d => s"${listType}.${s"${d.head.toUpper}${d.tail}"}")
+ trimmedDefault.split("\\s+").map(d => s"${listType}.${d.head.toUpper +: d.tail}")
Review Comment:
As above, this one is not just concatenating two strings.
So in this case a single string interpolation is the right thing.
```
s"${listType}.${d.head.toUpper}${d.tail}"
```
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/PropertyGenerator.scala:
##########
@@ -762,7 +762,7 @@ import org.apache.daffodil.lib.exceptions.ThrowsSDE
def getGeneratedFilePath(rootDir: String, pkg: String, filename: String): String = {
val outDir = new java.io.File(rootDir + "/" + pkg.split('.').reduceLeft(_ + "/" + _))
outDir.mkdirs()
- val outPath = s"$outDir/$filename"
+ val outPath = String.valueOf(outDir) + "/" + filename
Review Comment:
This one is not just concatenating two strings, so I think this is better as a string interpolation, particularly as it avoids the String.valueOf call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945157787
##########
build.sbt:
##########
@@ -276,18 +276,24 @@ def buildScalacOptions(scalaVersion: String) = {
"-language:experimental.macros",
"-unchecked",
"-Xfatal-warnings",
- "-Xxml:-coalescing",
- "-Xfuture",
- "-Ywarn-infer-any",
- "-Ywarn-inaccessible",
- // "-Ywarn-nullary-unit", // we cannot use this. It interferes with the Uniform Access Principle.
- // See https://stackoverflow.com/questions/7600910/difference-between-function-with-parentheses-and-without.
- "-Ywarn-unused-import"
+ "-Xxml:-coalescing"
)
val scalaVersionSpecificOptions = CrossVersion.partialVersion(scalaVersion) match {
case Some((2, 12)) =>
Seq(
+ "-Ywarn-unused:imports",
+ "-Xfuture",
+ "-Ywarn-infer-any",
+ "-Ywarn-inaccessible"
+ // "-Ywarn-nullary-unit", // we cannot use this. It interferes with the Uniform Access Principle.
+ // See https://stackoverflow.com/questions/7600910/difference-between-function-with-parentheses-and-without.
+ )
+ case Some((2, 13)) =>
+ Seq(
+ "-Xlint:inaccessible",
+ "-Xlint:infer-any",
+ "-Xlint:nullary-unit",
"-Ywarn-unused:imports"
Review Comment:
`-Ywarn-unused:imports` is in both 2.12 and 2.13, should that be moved to the above `commonOptions`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945200241
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/Implicits.scala:
##########
@@ -28,7 +28,7 @@ import org.apache.daffodil.lib.xml.NS
object Implicits {
object ImplicitsSuppressUnusedImportWarning {
- def apply() = if (scala.math.random.isNaN()) Assert.impossible()
+ def apply() = if (scala.math.random().isNaN) Assert.impossible()
Review Comment:
Put the code coverage suppression comments around these.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -57,7 +57,7 @@ object XercesValidatorFactory {
if (config.hasPath(XercesValidator.name))
config.getStringList(XercesValidator.name).asScala
else Seq.empty
- XercesValidator.fromFiles(schemaFiles)
+ XercesValidator.fromFiles(schemaFiles.toSeq)
Review Comment:
Ok, so what exactly are the rules about when you must have the "()" and when not?
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -352,12 +352,12 @@ class TunableEnumDefinition(
""".trim.stripMargin
private val scalaEnums = {
- val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
+ val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" }
scalaEnumValues.map { e => s""" case object ${e} extends ${scalaType}""" }
}
private val values = {
- val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
+ val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" }
Review Comment:
I'd prefer string concat not be done by string interpolations.
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -352,12 +352,12 @@ class TunableEnumDefinition(
""".trim.stripMargin
private val scalaEnums = {
- val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
+ val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" }
Review Comment:
If you are just concatenating strings then using "+" is clearer than the string interpolation which I must read very carefully to see if a whitespace is in there, or some other small characters.
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -316,7 +316,7 @@ class TunableEnumDefinition(
simpleTypeNode: scala.xml.Node
) {
private val nodeName = (simpleTypeNode \@ "name").stripPrefix("Tunable")
- private val scalaType = nodeName.head.toUpper + nodeName.tail
+ private val scalaType = s"${nodeName.head.toUpper}${nodeName.tail}"
Review Comment:
The string concat with + was clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix deprecated auto application of () to method invocations [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1422:
URL: https://github.com/apache/daffodil/pull/1422#discussion_r1945377011
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -352,12 +352,12 @@ class TunableEnumDefinition(
""".trim.stripMargin
private val scalaEnums = {
- val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail }
+ val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" }
Review Comment:
Calling toString is not, But `e.head.toUpper +: e.tail` is preferable to the string interpolation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org