feat: add functions (min, max, to_array, to_string, to_number, not_null, reverse) to JMESPath visitor #952
Conversation
0marperez
added
the
no-changelog
| "max" -> { | ||
| writer.addImport(RuntimeTypes.Core.Utils.max) | ||
| val list = expression.singleArg() | ||
| list.dotFunction(expression, "max()") | ||
| } | ||
|
|
||
| "min" -> { | ||
| writer.addImport(RuntimeTypes.Core.Utils.min) | ||
| val list = expression.singleArg() | ||
| list.dotFunction(expression, "min()") | ||
| } | ||
|
|
||
| "reverse" -> { | ||
| val listOrString = expression.singleArg() | ||
| listOrString.dotFunction(expression, "reversed()") | ||
| } |
There was a problem hiding this comment.
Question: Do these not need null guards?
There was a problem hiding this comment.
The dotFunction function takes care of that:
private fun VisitedExpression.dotFunction(expression: FunctionExpression, expr: String, elvisExpr: String? = null, isObject: Boolean = false): VisitedExpression {
val dotFunctionExpr = ensureNullGuard(shape, expr, elvisExpr)
...
}
| private fun List<VisitedExpression>.getNotNull(): String { | ||
| val answer = bestTempVarName("answer") | ||
| writer.write("var $answer: Any? = null") | ||
|
|
||
| val found = bestTempVarName("found") | ||
| writer.write("var $found = false") | ||
|
|
||
| forEach { | ||
| writer.withBlock("if (${it.identifier} != null && !$found) {", "}") { | ||
| write("$answer = ${it.identifier}") | ||
| write("$found = true") | ||
| } | ||
| } | ||
|
|
||
| return answer | ||
| } |
There was a problem hiding this comment.
Suggestion: This should be possible without mutability and intermediate variables like found:
private fun List<VisitedExpression>.getNotNull(): String {
val notNull = bestTempVarName("notNull")
writer.withBlock("val $notNull = listOfNotNull(", ").firstOrNull()") {
write("${it.identifier},")
}
return notNull
}Which produces code like:
val notNull1 = listOfNotNull(
foo,
bar,
baz,
).firstOrNull()| @InternalApi | ||
| public fun List<Number>.max(): Double? = | ||
| @Suppress("UNCHECKED_CAST") | ||
| (this as List<Double>).maxOrNull() |
There was a problem hiding this comment.
Question: Why is casting to Double correct here? Shouldn't the max of [1L, 2L, 3L] be 3L instead of 3.0?
| @InternalApi | ||
| public fun Number.toNumber(): Double = this.toDouble() | ||
|
|
||
| @InternalApi | ||
| public fun String.toNumber(): Double? = this.toDoubleOrNull() |
There was a problem hiding this comment.
Question: Same as above, why is Double always right here?
There was a problem hiding this comment.
So other parts of the code require a specific type of number and I chose double because I think all the number types we deal with can be represented as a Double (as long as they are small enough)
There was a problem hiding this comment.
There's also the problem of getting a number as a string, I'm not 100% sure but I think I have to choose a type of number for it when converting it to a number
| @@ -35,3 +35,24 @@ public fun truthiness(value: Any?): Boolean = when (value) { | |||
| null -> false | |||
| else -> true | |||
| } | |||
|
|
|||
| @InternalApi | |||
| public fun Short.toNumber(): Short = this | |||
There was a problem hiding this comment.
Can we move/group all the JMESPath related utilities into their own file JMESPath.kt (including ones introduced previously), it will help with understanding why these things exist and where they are used (should be used).
| public fun String.toNumber(): Double? = this.toDoubleOrNull() | ||
|
|
||
| @InternalApi | ||
| public fun Any.toNumber(): Nothing? = null |
There was a problem hiding this comment.
Why Nothing? as return type? Seems like Number? would be closer to correct.
There was a problem hiding this comment.
According to the JMESPath docs for to_number if it's used on anything other than a string or number it should return null. And from looking at the Kotlin docs for Nothing :
The nullable variant of this type, Nothing?, has exactly one possible value, which is null
|
Kudos, SonarCloud Quality Gate passed!
|
Issue #
Description of changes
-Added min function
-Added max function
-Added to_array function
-Added to_string function
-Added to_number function
-Added not_null function
-Added reverse funtion
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.