Skip to content
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.

Sign up for GitHub

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

Merge some of the WordPress lint rules, to reduce number of reported issues #21256

Merged
merged 11 commits into from
Nov 7, 2024
Merged

Merge some of the WordPress lint rules, to reduce number of reported issues #21256

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e210382
temp: use wip version of wordpress lint checks
wzieba Sep 24, 2024
47bae6b
Revert "temp: use wip version of wordpress lint checks"
wzieba Nov 1, 2024
53af796
Merge branch 'trunk' into bump_android_lint
wzieba Nov 1, 2024
3f5fa72
Don't enable `UnknownNullness` lint warning
wzieba Nov 1, 2024
2d61b81
Merge branch 'trunk' into bump_android_lint
wzieba Nov 1, 2024
20cd7bd
Disable disabling `UseTomlInstead` in `login` module
wzieba Nov 4, 2024
543395f
Merge branch 'trunk' into bump_android_lint
wzieba Nov 5, 2024
32b261d
Use version catalog for dependencies that specify artifact type
wzieba Nov 7, 2024
d89ed5e
Remove `UseTomlInstead` from disabled lint rules
wzieba Nov 7, 2024
94773c7
Remove `IgnoreWithoutReason` from disabled lint rules
wzieba Nov 7, 2024
ffb9840
Merge branch 'trunk' into bump_android_lint
wzieba Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion WordPress/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,6 @@ android {
checkGeneratedSources = true
lintConfig file("${project.rootDir}/config/lint/lint.xml")
baseline file("${project.rootDir}/config/lint/baseline.xml")
enable += 'UnknownNullness'
sarifReport = System.getenv('CI') ? true : false
}

Expand Down
7 changes: 0 additions & 7 deletions config/lint/lint.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,6 @@
</issue>
<issue id="IconDensities" severity="warning" /> <!-- fluxc-->
<!-- INTEROPERABILITY -->
<issue id="UnknownNullness" severity="informational">
<ignore path="**/generated/**" />
</issue>

<!-- TODO: https://github.com/wordpress-mobile/WordPress-Android/issues/21065 -->
<issue id="UsingMaterialAndMaterial3Libraries" severity="warning" />
<!-- We rely on more granural checks from WordPress-Lint-Android -->
<issue id="UnknownNullness" severity="ignore" />

</lint>
4 changes: 0 additions & 4 deletions libs/analytics/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,4 @@ android {
targetSdkVersion rootProject.targetSdkVersion
compileSdk rootProject.compileSdkVersion
}

lint {
enable += 'UnknownNullness'
}
}
4 changes: 0 additions & 4 deletions libs/editor/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ android {
compileSdk rootProject.compileSdkVersion
}

lint {
enable += 'UnknownNullness'
}

// Avoid 'duplicate files during packaging of APK' errors
packagingOptions {
exclude 'LICENSE.txt'
Expand Down
1 change: 0 additions & 1 deletion libs/fluxc/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ android {
}

lint {
enable += 'UnknownNullness'
disable 'UseTomlInstead', 'IgnoreWithoutReason'
}

Expand Down
4 changes: 0 additions & 4 deletions libs/image-editor/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ android {
}
}

lint {
enable += 'UnknownNullness'
}

buildFeatures {
viewBinding true
}
Expand Down
1 change: 0 additions & 1 deletion libs/login/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ android {
buildConfig true
}
lint {
enable += 'UnknownNullness'
disable 'UseTomlInstead'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (💡): I understand that we could remove this here too, now that all dependencies within this module are migrated to version catalogs. And then get rid of the whole lint { ... } block, just like you did for the rest of the modules.

PS: We could potentially do the same for the fluxc module, but we would first need to migrate the below two dependencies to version catalogs (however, I am not sure about the IgnoreWithoutReason):

    api "com.goterl:lazysodium-android:5.0.2@aar"
    api "net.java.dev.jna:jna:5.15.0@aar"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: We could potentially do the same for the fluxc module, but we would first need to migrate the below two dependencies to version catalogs

This is not easy to do though, because of the aar specification not working as intended. This is needed, because both of these dependencies offer both aar and jar and one of them even defaults to jar. This seems to be a known issue at Gradle (gradle/gradle#21267) but not yet fixed.

But regarding the login module, I agree, I'll fix this in a sec!

Copy link
Contributor

@ParaskP7 ParaskP7 Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But regarding the login module, I agree, I'll fix this in a sec!

Thanks @wzieba ! 🙏

This seems to be a known issue at Gradle (gradle/gradle#21267) but not yet fixed.

Yea, true, I tried really quick myself and couldn't get it working (the right way), but I didn't thought this would be such a blocker, interesting, thanks for sharing this issue... 🤔

I guess, in the meanwhile, we could do the following, adding a TODO comment point to the issue you shared:

libs.version.toml

...
[versions]
terl-lazysodium-android = '5.0.2@aar'
jna = '5.15.0@aar'
...
[libraries]
terl-lazysodium-android = { module = "com.goterl:lazysodium-android", version.ref = "terl-lazysodium-android" }
jna = { module = "net.java.dev.jna:jna", version.ref = "jna" }
...

build.gradle

api(libs.terl.lazysodium.android.get().toString()) // TODO: https://github.com/gradle/gradle/issues/21267
api(libs.jna.get().toString()) // TODO: https://github.com/gradle/gradle/issues/21267

Wdyt? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks neat, but I'm unable to verify that aar instead of jar is actually used.

Let's take the JNA dependency for the example: it offers both aar and jar. The default artifact specified in the pom file is jar: https://mvnrepository.com/artifact/net.java.dev.jna/jna/5.15.0

When I applied your suggestion and when I run :WordPress:dependencyInsight --dependency net.java.dev.jna:jna --configuration wordpressWasabiDebugRuntimeClasspath, it seems that jar is used. You can compare this result to result of any other dependency, e.g. :WordPress:dependencyInsight --dependency androidx.credentials:credentials --configuration wordpressWasabiDebugRuntimeClasspath which prints usage of aar.

But what's strange, when I run the dependency insight for JNA on trunk, I also see that jar is requested 😕 .

For this reason, as I'm not entirely sure how it all work, I'd postpone changing this dependencies to version catalog as I'm not sure what would be the effect. Happy to hear your thoughts - maybe there's a way to make sure aar is used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wzieba , I did a simple println(...) to verify the exact string that is used. In the end, using this .get().toString() trick should give us exactly what we had before, a simple string, there shouldn't be any more magic involved, not having version catalogs choosing between aar and jar whatsoever (🤔). This is my assumption at least. 🤞

println("LazySodium: ${libs.terl.lazysodium.android.get().toString()}")
println("JNA: ${libs.jna.get().toString()}")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha 👍 when I removed @aar the project won't compile, and with the code you suggested, build is complete so I think we're all good 👍

I've also applied changed to remove IgnoreWithoutReason from disabled rules.

}
}
Expand Down
4 changes: 0 additions & 4 deletions libs/mocks/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ android {
targetSdkVersion rootProject.targetSdkVersion
compileSdk rootProject.compileSdkVersion
}

lint {
enable += 'UnknownNullness'
}
}

dependencies {
Expand Down
4 changes: 0 additions & 4 deletions libs/networking/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ android {
targetSdkVersion rootProject.targetSdkVersion
compileSdk rootProject.compileSdkVersion
}

lint {
enable += 'UnknownNullness'
}
}

dependencies {
Expand Down
Loading