Feature: Lint Checks #158
Feature: Lint Checks #158
Conversation
| // According to https://github.com/googlesamples/android-custom-lint-rules/tree/master/android-studio-3 | ||
| // the lint version should match to the used Android Gradle Plugin by the formula "AGP Version X.Y.Z + 23.0.0" | ||
| // E.g. "AGP Version 3.1.3 + 23.0.0 = Lint Version 26.1.3" | ||
| lintVersion = '26.1.3' |
There was a problem hiding this comment.
We could also calculate this by ourselfs, right? 🤔
Just extract the AGP version and "add" the values to 23.0.0 ...
There was a problem hiding this comment.
Yes, we can calculate this ourself. But I would prefer to change that still manually:
- It is stated "should" and so the actual version might differ from the formula
- Google recently just decoupled the Support Lib Versions and so it might be that this version gets uncoupled from the build tools version aswell
thirtyinch-lint/README.md
Outdated
| @@ -0,0 +1 @@ | |||
| # ThirtyInch - Lint | |||
There was a problem hiding this comment.
No one of our modules have its own README.
Even if this brings no value I prefer to remove it
thirtyinch-lint/build.gradle
Outdated
| compileOnly "com.android.tools.lint:lint-api:$lintVersion" | ||
| compileOnly "com.android.tools.lint:lint-checks:$lintVersion" | ||
|
|
||
| testImplementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8" |
There was a problem hiding this comment.
This dependency is already implement as a implementation dependency.
Because testImplementation extends implementation there is no need to declare it again
thirtyinch-lint/build.gradle
Outdated
|
|
||
| sourceCompatibility = 1.6 | ||
|
|
||
| defaultTasks 'assemble' |
There was a problem hiding this comment.
Is that really needed? 🤔
I prefer to remove it.
Running gradle without any specific task is not that common...
thirtyinch-lint/build.gradle
Outdated
| } | ||
| } | ||
|
|
||
| sourceCompatibility = 1.6 |
| "com.pascalwelsch.compositeandroid.fragment.CompositeFragment" | ||
| ) | ||
|
|
||
| class MissingViewInCompositeDetector : BaseMissingViewDetector() { |
There was a problem hiding this comment.
Don't know if it make sense because we wanted to drop the support vor CompositeAndroid anyway, right @passsy?
But as long as it is implemented now and work we could leave it like it is...
There was a problem hiding this comment.
Correct, as long it is still present we should support it.
|
|
||
| override fun applicableSuperClasses() = TI_CLASS_NAMES | ||
|
|
||
| override val issue: Issue; get() = ISSUE |
There was a problem hiding this comment.
Why is here an semicolon?
Should be removed. See also this style guide.
There was a problem hiding this comment.
This semicolon is needed to keep it all in one line. But I removed the getter overriding completely.
| companion object { | ||
| val ISSUE = MissingView.asLintIssue( | ||
| MissingViewInThirtyInchDetector::class.java, | ||
| "When using ThirtyInch, a class extending TiActivity, TiFragment or CompositeActivity " + |
There was a problem hiding this comment.
This issue don't catch the issues for the CompositeActivity.
Please adjust the comment
|
@StefMa I addressed all your comments. Would be great if you could have another look on this PR. :) |
|
Hey, sure. |
| private const val PROVIDE_VIEW_METHOD = "provideView" | ||
| private val TI_CLASS_NAMES = listOf( | ||
| "net.grandcentrix.thirtyinch.TiActivity", | ||
| "net.grandcentrix.thirtyinch.TiFragment" |
There was a problem hiding this comment.
We have a TiDialogFragment which will not be noticed as issue.
Please add it 👍
|
Works 🎉 |
|
What does not work: No Lint issue will be shown :( |
|
Works also with Java 🤘 |
|
Something like this shouldn't be an issue: Ok, maybe this is not an issue because it will never happen in real world |
| from(configurations.lintChecks) { | ||
| rename { String fileName -> 'lint.jar' } | ||
| } | ||
| into 'build/intermediates/lint/' |
There was a problem hiding this comment.
Is it common to move the lint check into the final AAR? 🤔
What about to publish a separate jar/artifact and make this optional? 🤔
There was a problem hiding this comment.
Having custom lint checks for a library is only really possible since AGP v3.0. Before you had to manually copy a lint.jar to specific folder and so on.
So not a lot libs ship their own lint checks with the AAR atm.
But in my view it is way more convenient if you have them out-of-the-box. If you do not want them, you could deactivate them in the Android Studio preferences anyway.
|
Other than that it looks great 🎉 |
|
Thanks for the extensive tests! 😃 |
In my view this is correct. StuffActivity could be also used separately and so we should highlight this error. |
OK, wasn't that hard than expected. This scenario should also be covered now. |
|
All done. |
|
Issue created as discussed. Thank you @jreehuis 🤘 |
Description
This implementation adopts and updates #111.
Fixes #100
The updates include:
TiIssueis now a sealed classHow to Test
Play with the following scenarios:
implements HelloWorldViewin theHelloWorldActivityof the sample appimplements HelloWorldViewbut overrideprovideView()in theHelloWorldActivityof the sample app0.9.1-SNAPSHOTof this branch in another project and have an TiActivity which does not implement the givenTiViewsubclass