Feature: Lint Checks by jreehuis · Pull Request #158 · GCX-HCI/ThirtyInch

jreehuis · 2018-08-27T13:39:11Z

Description

This implementation adopts and updates #111.
Fixes #100

The updates include:

Using the now stable Lint version to use UAST
Bundles the Lint checks with TI
Fixing the detection scope so Android Studio will report the error in the editor
Creating an own TI Lint Category
TiIssue is now a sealed class
some Kotlin tweaks

How to Test

Play with the following scenarios:

Remove the implements HelloWorldView in the HelloWorldActivity of the sample app
Remove the implements HelloWorldView but override provideView() in the HelloWorldActivity of the sample app
Use TI v0.9.1-SNAPSHOT of this branch in another project and have an TiActivity which does not implement the given TiView subclass

StefMa

Haven't testet it yet.
But code looks good so far 👌

StefMa · 2018-08-28T06:39:22Z

+    // 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'


We could also calculate this by ourselfs, right? 🤔
Just extract the AGP version and "add" the values to 23.0.0 ...

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

StefMa · 2018-08-28T06:39:57Z

@@ -0,0 +1 @@
+# ThirtyInch - Lint


No one of our modules have its own README.
Even if this brings no value I prefer to remove it

StefMa · 2018-08-28T06:40:42Z

+    compileOnly "com.android.tools.lint:lint-api:$lintVersion"
+    compileOnly "com.android.tools.lint:lint-checks:$lintVersion"
+
+    testImplementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8"


This dependency is already implement as a implementation dependency.
Because testImplementation extends implementation there is no need to declare it again

StefMa · 2018-08-28T06:42:07Z

+
+sourceCompatibility = 1.6
+
+defaultTasks 'assemble'


Is that really needed? 🤔
I prefer to remove it.
Running gradle without any specific task is not that common...

StefMa · 2018-08-28T06:42:32Z

+    }
+}
+
+sourceCompatibility = 1.6


Why do we need this?

StefMa · 2018-08-28T06:45:50Z

+        "com.pascalwelsch.compositeandroid.fragment.CompositeFragment"
+)
+
+class MissingViewInCompositeDetector : BaseMissingViewDetector() {


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...

Correct, as long it is still present we should support it.

StefMa · 2018-08-28T06:47:15Z

+
+    override fun applicableSuperClasses() = TI_CLASS_NAMES
+
+    override val issue: Issue; get() = ISSUE


Why is here an semicolon?
Should be removed. See also this style guide.

This semicolon is needed to keep it all in one line. But I removed the getter overriding completely.

StefMa · 2018-08-28T06:48:07Z

+    companion object {
+        val ISSUE = MissingView.asLintIssue(
+                MissingViewInThirtyInchDetector::class.java,
+                "When using ThirtyInch, a class extending TiActivity, TiFragment or CompositeActivity " +


This issue don't catch the issues for the CompositeActivity.
Please adjust the comment

jreehuis · 2018-09-12T09:30:22Z

@StefMa I addressed all your comments. Would be great if you could have another look on this PR. :)

StefMa · 2018-09-12T11:48:56Z

Hey, sure.
After I am back from my sick leave I will take a look into it 👍

StefMa · 2018-09-17T08:25:20Z

+private const val PROVIDE_VIEW_METHOD = "provideView"
+private val TI_CLASS_NAMES = listOf(
+        "net.grandcentrix.thirtyinch.TiActivity",
+        "net.grandcentrix.thirtyinch.TiFragment"


We have a TiDialogFragment which will not be noticed as issue.
Please add it 👍

StefMa · 2018-09-17T08:25:41Z

Works 🎉

StefMa · 2018-09-17T08:27:41Z

What does not work:

abstract class GalleryDetailsActivity : TiActivity<GalleryDetailsPresenter, GalleryDetailsView>()

class CameraGalleryDetailsActivity : GalleryDetailsActivity() {

No Lint issue will be shown :(

StefMa · 2018-09-17T08:32:11Z

Works also with Java 🤘

StefMa · 2018-09-17T08:33:23Z

Something like this shouldn't be an issue:

Ok, maybe this is not an issue because it will never happen in real world

StefMa · 2018-09-17T08:37:18Z

+    from(configurations.lintChecks) {
+        rename { String fileName -> 'lint.jar' }
+    }
+    into 'build/intermediates/lint/'


Is it common to move the lint check into the final AAR? 🤔
What about to publish a separate jar/artifact and make this optional? 🤔

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.

StefMa · 2018-09-17T08:39:06Z

Other than that it looks great 🎉
Thanks @jreehuis

jreehuis · 2018-09-17T08:40:29Z

Thanks for the extensive tests! 😃
Especially the abstract class scenario will get complicated... 😡
I will see to solve it. But definitely a real world case. Thanks for pointing to it.

jreehuis · 2018-09-17T11:35:23Z

Something like this shouldn't be an issue:

Ok, maybe this is not an issue because it will never happen in real world

In my view this is correct. StuffActivity could be also used separately and so we should highlight this error.

jreehuis · 2018-09-17T11:39:37Z

What does not work:

abstract class GalleryDetailsActivity : TiActivity<GalleryDetailsPresenter, GalleryDetailsView>()

class CameraGalleryDetailsActivity : GalleryDetailsActivity() {

No Lint issue will be shown :(

OK, wasn't that hard than expected. This scenario should also be covered now.

jreehuis · 2018-09-17T11:39:48Z

All done.

StefMa · 2018-09-24T13:03:03Z

Issue created as discussed.
Merged ✅

Thank you @jreehuis 🤘

jreehuis-gcx added 9 commits August 1, 2018 14:13

initial setup of the thirty inch-lint module

81221da

first Implementation for Java inspired by #111

b6bfdad

configures gradle to include the TI lint checks in the TI lib

616ad0b

fixes not executed lint checks in AS

770dfd6

create own TI lint category

040e117

improves the highlighted editor element for the TiViewMissing lint issue

aebadbe

some comment cleanup

0166387

adds comments about the correct lintVersion

d88c24c

also register the Lint-Registry-v2

2b8e3f0

jreehuis added the Ready for Review label Aug 27, 2018

StefMa suggested changes Aug 28, 2018

View reviewed changes

jreehuis-gcx added 6 commits August 28, 2018 10:35

removes the empty README.md

6fe43a9

removes obsolete kotlin stdlib inclusion

3d43e9b

removes obsolete defaultTasks

9b34699

removes obsolete sourceCompatibility

7b5ac3e

improves the issue val overriding syntax

da0b916

corrects comment

fa159cb

jreehuis mentioned this pull request Sep 12, 2018

Feature: getViewOrThrow() in onAttachView() Lint-Check #160

Closed

jreehuis added the lint label Sep 12, 2018

jreehuis-gcx added 7 commits September 13, 2018 11:04

adds Kolin Tests

dfafaa0

adds a more complex inheritance scenario as test

a0ce173

fixes false positive when having a base presenter class

d339edf

adds missing lint version specification

fd26e60

fixes wrong test conditions

38892fa

also implements the basePresenter tests for the fragment

d641968

Improves the TI Category priority to align with Category.CORRECTNESS

620feca

StefMa reviewed Sep 17, 2018

View reviewed changes

jreehuis-gcx added 4 commits September 17, 2018 11:01

makes the findViewInterface method signature more abstract

ce5da87

implements more robust view type determination

ea76784

adds more tests for the better view type implementation

bf9f5c2

enables the ViewNotImplementedCheck for the TiDialogFragment

139f3f8

StefMa mentioned this pull request Sep 24, 2018

Lint does not report by abstract classes without implementation View #162

Open

StefMa approved these changes Sep 24, 2018

View reviewed changes

StefMa merged commit d761dbe into GCX-HCI:master Sep 24, 2018

mannodermaus mentioned this pull request Sep 25, 2018

Added Lint Check for missing TiView implementation #111

Closed

jreehuis deleted the feature/lint_checks branch February 19, 2019 09:01


		override fun applicableSuperClasses() = TI_CLASS_NAMES

		override val issue: Issue; get() = ISSUE


		sourceCompatibility = 1.6

		defaultTasks 'assemble'

Conversation

jreehuis commented Aug 27, 2018 • edited by StefMa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to Test

Uh oh!

StefMa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreehuis commented Sep 12, 2018

Uh oh!

StefMa commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefMa commented Sep 17, 2018

Uh oh!

StefMa commented Sep 17, 2018

Uh oh!

StefMa commented Sep 17, 2018

Uh oh!

StefMa commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefMa commented Sep 17, 2018

Uh oh!

jreehuis commented Sep 17, 2018

Uh oh!

jreehuis commented Sep 17, 2018

Uh oh!

jreehuis commented Sep 17, 2018

Uh oh!

jreehuis commented Sep 17, 2018

Uh oh!

StefMa commented Sep 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

jreehuis commented Aug 27, 2018 •

edited by StefMa

Loading

StefMa commented Sep 17, 2018 •

edited

Loading