-
Notifications
You must be signed in to change notification settings - Fork 498
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.
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
Replace custom binding parameter in INSERT too #3569
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic looks good, change requested on some naming stuff but feel free to merge and follow up or I can make that change
@@ -5,7 +5,7 @@ import com.intellij.lang.ASTNode | |||
|
|||
abstract class BindParameterMixin(node: ASTNode) : BindParameterMixin(node) { | |||
override fun replaceWith(isAsync: Boolean, index: Int): String = when { | |||
isAsync -> "$$index" | |||
isAsync -> "$${index + 1}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like this is a weird implementation detail, like is it always the case that async
is what causes us to use indexed parameters instead of unindexed? I think I would prefer this function just takes a nullable index as the parameter and then the actual place where async drivers want to replace the index they can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and have it default to null so a bunch of callsites could be cleaned up maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what you want. I agree this case is special, but what do you mean with nullable index?
override fun replaceWith(index: Int? = null): String = index?.let { "$${it + 1}" } ?: "?"
What about postgres without async then? Or do you want to move this into the compiler? 🤔
@@ -151,7 +153,7 @@ inline fun <reified T : PsiElement> PsiElement.nextSiblingOfType(): T { | |||
return PsiTreeUtil.getNextSiblingOfType(this, T::class.java)!! | |||
} | |||
|
|||
private fun PsiElement.rangesToReplace(): List<Pair<IntRange, String>> { | |||
private fun PsiElement.rangesToReplace(isAsync: Boolean): List<Pair<IntRange, String>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this parameter should probably be alwaysUseIndexedVariables
or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ie I want our parameters to clearly state the change they want to happen, vs stating the state they're currently in and having an implementation detail gate its logic on that, as much as possible
and if it truly is logic thats gated on some global state of the compiler, we should be querying it from one of the project structure types or something like that instead of passing it around.
Fixes #3564