-
Notifications
You must be signed in to change notification settings - Fork 419
Question marks in a query don't match number of arguments lint check #220
Conversation
Thanks! I'll take a look soon here. If you're feeling generous you can rebase on the latest |
Rebased generously :) |
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.
This is great. I only have some small nits.
@@ -0,0 +1,23 @@ | |||
/* | |||
* Copyright (C) 2015 Square, Inc. |
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.
2017
@@ -0,0 +1,74 @@ | |||
/* | |||
* Copyright (C) 2015 Square, Inc. |
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.
2017
"Number of provided arguments doesn't match number " + | ||
"of arguments specified in query", | ||
"When providing arguments to query you need to provide the same amount of" + | ||
"arguments that is specified in query.", |
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.
nit: missing space between "of" and "arguments" inside the concatenation. I would prefer it after "of" similar to the above string.
val evaluator = context.evaluator | ||
|
||
if (evaluator.isMemberInClass(method, BRITE_DATABASE)) { | ||
// skip non varargs overloads |
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.
super nit: can you capitalize your comments and add periods? comments should be sentences.
val questionMarksCount = sql.count { it == '?' } | ||
if (argumentsCount != questionMarksCount) { | ||
context.report(ISSUE, call, context.getLocation(call), "Wrong argument count, " + | ||
"query $sql requires $questionMarksCount arguments, but was provided" + |
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.
can you hoist the "arguments" word here and on the next line out to two local variables and check whether or not the respective count variable is 1. If so, make it the singular "argument" and if it's not 1 make it "arguments". It's a small detail, but an easy one to fix.
@@ -0,0 +1,203 @@ | |||
/* | |||
* Copyright (C) 2015 Square, Inc. |
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.
2017
settings.gradle
Outdated
@@ -1,5 +1,6 @@ | |||
include ':sqlbrite' | |||
include ':sqlbrite-kotlin' | |||
include ':sample' | |||
include ':checks' |
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.
nit: can you name this 'lint-checks' or 'sqlbrite-lint'? 'checks' is a bit vague.
@jrodbx when you get back from vacation can you take a quick look at this? |
Cool! Thanks for the review. |
Category.MESSAGES, | ||
9, | ||
Severity.ERROR, | ||
Implementation(SqlBriteArgCountDetector::class.java, Scope.JAVA_FILE_SCOPE)) |
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.
We probably also want to run this lint check in test sources. This can be done using EnumSet.of(JAVA_FILE, TEST_SOURCES))
|
||
private fun UCallExpression.isQueryMethod() = methodName == QUERY_METHOD_NAME | ||
|
||
private fun String.pluralize(count: Int) = if (count == 1) this else this + "s" |
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.
There's already this function in LintUtils.pluralize but unfortuantely it's private :( we could request it to be made public
class SqlBriteArgCountDetectorTest : LintDetectorTest() { | ||
|
||
companion object { | ||
private val BRITE_DATABASE_STUB = TestFiles.java( |
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.
nit: also statically import?
import com.android.tools.lint.checks.infrastructure.LintDetectorTest | ||
import com.android.tools.lint.checks.infrastructure.TestFiles | ||
|
||
class SqlBriteArgCountDetectorTest : LintDetectorTest() { |
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.
since you're using the new way of testing by using lint()
you no longer need to extend from LintDetectorTest
however you'll then need to use Junit4 and add the @test annotation on all of the methods
|
||
override fun getIssues() = listOf(SqlBriteArgCountDetector.ISSUE) | ||
|
||
fun testCleanCaseWithWithQueryAsLiteral() { |
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.
Here it's this then:
@Test fun cleanCaseWithWithQueryAsLiteral() {
} | ||
""".trimIndent())) | ||
.run() | ||
.expect("src/test/pkg/Test.java:11: " + |
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.
You could use the """ here with trimMargin() similiar to:
.expect("""
|res/layout/random.xml: Warning: Layout does not start with one of the following prefixes: activity_, view_, fragment_, dialog_, bottom_sheet_, adapter_item_, divider_, space_ [WrongLayoutName]
|0 errors, 1 warnings
""".trimMargin())
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 don't know, I think that I got an idea how the function works, but I just can't make it work. Tbh, not even sure if it brings more readability than regular string concatenation in that case
@@ -65,11 +63,14 @@ class SqlBriteArgCountDetectorTest : LintDetectorTest() { | |||
|
|||
} | |||
""".trimIndent())) | |||
.detector(SqlBriteArgCountDetector()) |
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.
no need for passing the detctor as the issues function takes the Issue which has an implementation that links to SqlBriteArgCountDetector Implementation(SqlBriteArgCountDetector::class.java, EnumSet.of(JAVA_FILE, TEST_SOURCES)))
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.
LGTM with a few requests!
build.gradle
Outdated
lintChecks = "com.android.tools.lint:lint-checks:${versions.lint}" | ||
lint = "com.android.tools.lint:lint:${versions.lint}" | ||
lintTests = "com.android.tools.lint:lint-tests:${versions.lint}" | ||
lintTestUtils = "com.android.tools:testutils:${versions.lint}" |
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.
delete. testutils
is pulled in transitively by lint-tests
sqlbrite-lint/build.gradle
Outdated
|
||
testCompile rootProject.ext.junit | ||
testCompile rootProject.ext.lint | ||
testCompile rootProject.ext.lintTests |
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.
testCompile
=> testImplementation
now that this project is using Android Gradle Plugin 3.0.
sqlbrite-lint/build.gradle
Outdated
testCompile rootProject.ext.junit | ||
testCompile rootProject.ext.lint | ||
testCompile rootProject.ext.lintTests | ||
testCompile rootProject.ext.lintTestUtils |
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.
delete
|
||
// Position of sql parameter depends on method. | ||
val sql = evaluateString(context, | ||
call.valueArguments[if (call.isQueryMethod()) 0 else 1], true) ?: return |
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.
nice
import com.squareup.sqlbrite3.BriteDatabase; | ||
|
||
public class Test { | ||
|
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.
nit: remove blank line after each test class's opening brace/before QUERY declaration
Thanks for the review @jrodbx ! |
import com.android.tools.lint.detector.api.Severity | ||
import com.intellij.psi.PsiMethod | ||
import org.jetbrains.uast.UCallExpression | ||
import java.util.* |
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.
nit: expand this to individual imports. we don't allow star imports
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.
Some final small things from me. Otherwise let's get this merged.
settings.gradle
Outdated
@@ -1,5 +1,6 @@ | |||
include ':sqlbrite' | |||
include ':sqlbrite-kotlin' | |||
include ':sample' | |||
include ':sqlbrite-lint' |
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.
super nit: can you move this up so it's properly alphabetized with the other sqlbrite-prefixed modules. "sample" is intentionally last
apply plugin: 'kotlin' | ||
|
||
dependencies { | ||
compileOnly rootProject.ext.kotlinStdLib |
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.
This should be implementation
since, in theory, future versions of lint could not be based on Kotlin and thus not have the stdlib already on the classpath.
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.
If I mark it as implementation
instead of compileOnly
I have this error:
Execution failed for task :sqlbrite:prepareLintJar.
Found more than one jar in the 'lintChecks' configuration. Only one file is supported. If using a separate Gradle project, make sure compilation dependencies are using compileOnly
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.
That's... interesting. I mean it makes sense, but I guess we have to assume that lint will always contain the Kotlin stdlib as a result.
sqlbrite-lint/build.gradle
Outdated
dependencies { | ||
compileOnly rootProject.ext.kotlinStdLib | ||
compileOnly rootProject.ext.lintApi | ||
compileOnly rootProject.ext.lintChecks |
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.
@jrodbx We need both of these? Also is compileOnly
correct for both?
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.
At least in the official custom lint rule sample, they have it like that.
https://github.com/googlesamples/android-custom-lint-rules/blob/master/android-studio-3/checks/build.gradle
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.
compileOnly is correct for both, since the runtime will provide the classes; however, good catch on the unnecessary lintChecks
dependency. that's only needed when you're referencing/subclassing from the standard lint checks.
@geralt-encore it doesn't appear that you're relying on the standard lint checks, so you can remove the lintChecks
dependency.
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.
@jrodbx thanks for the explanation! Wasn't sure about this dependency
Thanks! |
#58
This is my first lint check so I am not sure if I've covered all the cases. I was using Timber lint checks as a reference. I'll appreciate any feedback!