Fix deprecated auto application of () to method invocations#1422
Fix deprecated auto application of () to method invocations#1422olabusayoT merged 1 commit intoapache:mainfrom
Conversation
| "-Xlint:inaccessible", | ||
| "-Xlint:infer-any", | ||
| "-Xlint:nullary-unit", | ||
| "-Ywarn-unused:imports" |
There was a problem hiding this comment.
-Ywarn-unused:imports is in both 2.12 and 2.13, should that be moved to the above commonOptions?
mbeckerle
left a comment
mbeckerle
left a comment
There was a problem hiding this comment.
I don't like replacing simple string concatenation by string interpolations.
|
|
||
| object ImplicitsSuppressUnusedImportWarning { | ||
| def apply() = if (scala.math.random.isNaN()) Assert.impossible() | ||
| def apply() = if (scala.math.random().isNaN) Assert.impossible() |
There was a problem hiding this comment.
Put the code coverage suppression comments around these.
| config.getStringList(XercesValidator.name).asScala | ||
| else Seq.empty | ||
| XercesValidator.fromFiles(schemaFiles) | ||
| XercesValidator.fromFiles(schemaFiles.toSeq) |
There was a problem hiding this comment.
Ok, so what exactly are the rules about when you must have the "()" and when not?
There was a problem hiding this 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
|
|
||
| private val scalaEnums = { | ||
| val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail } | ||
| val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" } |
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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 ?
There was a problem hiding this comment.
But to your point sticking with string concatenation will look something like
e/head.toUpper.toString + e.tail
Is that preferable?
There was a problem hiding this comment.
Calling toString is not, But e.head.toUpper +: e.tail is preferable to the string interpolation.
| ) { | ||
| private val nodeName = (simpleTypeNode \@ "name").stripPrefix("Tunable") | ||
| private val scalaType = nodeName.head.toUpper + nodeName.tail | ||
| private val scalaType = s"${nodeName.head.toUpper}${nodeName.tail}" |
There was a problem hiding this comment.
The string concat with + was clearer.
|
|
||
| private val values = { | ||
| val scalaEnumValues = allEnumerationValues.map { e => e.head.toUpper + e.tail } | ||
| val scalaEnumValues = allEnumerationValues.map { e => s"${e.head.toUpper}${e.tail}" } |
There was a problem hiding this comment.
I'd prefer string concat not be done by string interpolations.
mbeckerle
left a comment
mbeckerle
left a comment
There was a problem hiding this comment.
2 suggestions.
Plus, you also should suppress those codeCov things with their "COVERAGE-OFF" comment stuff.
| } 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}") |
There was a problem hiding this 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}"
| val outDir = new java.io.File(rootDir + "/" + pkg.split('.').reduceLeft(_ + "/" + _)) | ||
| outDir.mkdirs() | ||
| val outPath = s"$outDir/$filename" | ||
| val outPath = String.valueOf(outDir) + "/" + filename |
There was a problem hiding this 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.
- 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 + with interpolated strings or char prepend where appropriate - add coverage comments DAFFODIL-2152
olabusayoT
force-pushed
the
daf-2152-auto-application
branch
from
e5c12e0 to
4b7e02b
Compare
3 participants
DAFFODIL-2152