DOPE-258: added positionalParameters to DopeQuery and added it to each toDopeQuery() and test #56
Conversation
…h toDopeQuery() and test
There was a problem hiding this comment.
Overall great!
I do not really see the benefit in testing every expression with (almost) every combination of parameters; what do these tests really test, the function/expression itself or parameters? In my opinion a few build tests with parameters of different types would benefit us more, what do you think?
If we leave them in, I would request changing all the literal assignments in the tests to the helper functions.
| @@ -1,3 +1,3 @@ | |||
| package ch.ergon.dope | |||
|
|
|||
| data class DopeQuery(val queryString: String, val parameters: Map<String, Any>) | |||
| data class DopeQuery(val queryString: String, val parameters: Map<String, Any>, val positionalParameters: List<Any>) | |||
There was a problem hiding this comment.
I know requesting this will add even more files/lines to this PR:
I think we should rename parameters to namedParameters for better distinctiveness.
| @@ -27,6 +27,9 @@ class GroupByClause( | |||
| parameters = fieldsDopeQuery.fold(fieldDopeQuery.parameters) { fieldParameters, field -> | |||
| fieldParameters + field.parameters | |||
| } + parentDopeQuery.parameters, | |||
| positionalParameters = parentDopeQuery.positionalParameters + fieldsDopeQuery.fold( | |||
| fieldDopeQuery.positionalParameters, | |||
| ) { fieldParameters, field -> fieldParameters + field.positionalParameters }, | |||
There was a problem hiding this comment.
Please format the same as the named parameters above.
| @@ -30,6 +30,9 @@ sealed class ReturningClause( | |||
| parameters = fieldsDopeQuery.fold(fieldDopeQuery.parameters) { fieldParameters, field -> | |||
| fieldParameters + field.parameters | |||
| } + parentDopeQuery.parameters, | |||
| positionalParameters = parentDopeQuery.positionalParameters + fieldsDopeQuery.fold( | |||
| fieldDopeQuery.positionalParameters, | |||
| ) { fieldParameters, field -> fieldParameters + field.positionalParameters }, | |||
There was a problem hiding this comment.
Formatting as above, and I saw that you changed the order to always put the parentDopeQuery parameters first, please change everywhere or create a new ticket for that 🙂
| @@ -40,6 +41,7 @@ class SelectOrderByTypeClause( | |||
| return DopeQuery( | |||
| queryString = formatToQueryStringWithSymbol(parentDopeQuery.queryString, ORDER_BY, stringDopeQuery.queryString + " $orderByType"), | |||
| parameters = stringDopeQuery.parameters + parentDopeQuery.parameters, | |||
| positionalParameters = stringDopeQuery.positionalParameters + parentDopeQuery.positionalParameters, | |||
There was a problem hiding this comment.
Order of parameters (parent first)
| collection.map { it.toDopeQuery(manager) }.let { dopeQueries -> | ||
| DopeQuery( | ||
| queryString = formatListToQueryStringWithBrackets(dopeQueries, prefix = "[", postfix = "]"), | ||
| parameters = dopeQueries.fold(emptyMap()) { parameters, dopeQueryElement -> parameters + dopeQueryElement.parameters }, | ||
| positionalParameters = dopeQueries.fold(emptyList()) { positionalParameters, dopeQueryElement -> | ||
| positionalParameters + dopeQueryElement.positionalParameters | ||
| }, |
There was a problem hiding this comment.
Reformat both to be the same
| positionalParameters = firstStringDopeQuery.positionalParameters + secondStringDopeQuery.positionalParameters + | ||
| stringTypesDopeQuery.fold(emptyList()) { stringTypePositionalParameters, field -> | ||
| stringTypePositionalParameters + field.positionalParameters | ||
| }, |
| @@ -1,3 +1,3 @@ | |||
| package ch.ergon.dope | |||
|
|
|||
| data class DopeQuery(val queryString: String, val parameters: Map<String, Any>) | |||
| data class DopeQuery(val queryString: String, val parameters: Map<String, Any>, val positionalParameters: List<Any>) | |||
There was a problem hiding this comment.
What about if we hide the params a step deeper like:
data class DopeQuery(val queryString: String, val parameters: DopeParameters)
data class DopeParameters(val namedParameters: Map<String, Any>, val positionalParameters: List<Any>)
There was a problem hiding this comment.
And the we can provide a merge-function:
data class DopeParameters(val namedParameters: Map<String, Any>, val positionalParameters: List<Any>) {
fun merge(otherParams: DopeParameters) = this.copy(
)
}
which merges two DopeParameters.
| override fun toDopeQuery(manager: DopeQueryManager) = DopeQuery(formatIndexToQueryString(indexName, indexType?.type), emptyMap()) | ||
| override fun toDopeQuery(manager: DopeQueryManager) = DopeQuery( | ||
| queryString = formatIndexToQueryString(indexName, indexType?.type), | ||
| parameters = emptyMap(), |
There was a problem hiding this comment.
can we maybe make these emptyMaps and emptyList as default values?
| @@ -18,7 +18,8 @@ class FromClause(private val fromable: Fromable, private val parentClause: ISele | |||
| } | |||
| return DopeQuery( | |||
| queryString = formatToQueryStringWithSymbol(parentDopeQuery.queryString, "FROM", fromableDopeQuery.queryString), | |||
| parameters = fromableDopeQuery.parameters + parentDopeQuery.parameters, | |||
| parameters = parentDopeQuery.parameters + fromableDopeQuery.parameters, | |||
| positionalParameters = parentDopeQuery.positionalParameters + fromableDopeQuery.positionalParameters, | |||
There was a problem hiding this comment.
Can we merge positionalParamters like that? Let's assume parentDopeQuery and fromableDopeQuery have both two positionalParameters. If we merge it like that, then parentDopeQuery.queryString has $1 and $2 and fromableDopeQuery.queryString has $3 and $4 is this always guaranteed?
I am very sorry if you are reviewing this