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

wzieba · 2024-09-24T17:31:21Z

Description

This PR disables UnkownNullness lint check, to not duplicate lint warnings of the same type we get from WordPress-Lint.

Testing

Not necessary.

Related: wordpress-mobile/WordPress-Lint-Android#20

dangermattic · 2024-09-24T17:34:35Z

	1 Warning
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

sonarqubecloud · 2024-09-24T17:34:47Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

wpmobilebot · 2024-09-24T17:49:33Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr21256-ffb9840
	Commit	`ffb9840`
	Direct Download	`jetpack-prototype-build-pr21256-ffb9840.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-09-24T17:50:28Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr21256-ffb9840
	Commit	`ffb9840`
	Direct Download	`wordpress-prototype-build-pr21256-ffb9840.apk`

Note: Google Login is not supported on these builds.

ParaskP7 · 2024-09-25T07:32:36Z

config/lint/lint.xml

@@ -79,4 +79,6 @@

    <!-- 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" />


Suggestion (💡): Instead of ignoring UnknownNullness I actually recommend we revert #19116 and remove all the configuration related to that, effectively removing it, not ignoring. 🤔

Thank you @ParaskP7 ! I've applied your suggestion and rebranded this PR to not introduce changed WordPress Lint checks. I don't have capacity to work on this now, but I think PR in this shape will also be useful to merge!

This reverts commit e210382.

We have this covered via checks from WordPress-Lint

ParaskP7

👋 @wzieba !

I have reviewed this PR as per the instructions, everything works as expected, good job! 🌟

I have left one suggestion (💡) for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

ParaskP7 · 2024-11-04T10:59:25Z

libs/login/build.gradle

@@ -18,7 +18,6 @@ android {
        buildConfig true
    }
    lint {
-        enable += 'UnknownNullness'
        disable 'UseTomlInstead'


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"

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!

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? 🤔

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?

@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()}")

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.

Related discussion: #21256 (comment)

Suppress this warning on imported tests that are breaking this rule

sonarqubecloud · 2024-11-07T11:57:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

temp: use wip version of wordpress lint checks

e210382

ParaskP7 reviewed Sep 25, 2024

View reviewed changes

wzieba added 3 commits November 1, 2024 16:59

Revert "temp: use wip version of wordpress lint checks"

47bae6b

This reverts commit e210382.

Merge branch 'trunk' into bump_android_lint

53af796

Don't enable UnknownNullness lint warning

3f5fa72

We have this covered via checks from WordPress-Lint

wzieba added the Core label Nov 1, 2024

Merge branch 'trunk' into bump_android_lint

2d61b81

wzieba marked this pull request as ready for review November 1, 2024 16:06

wzieba requested a review from ParaskP7 November 1, 2024 16:07

ParaskP7 approved these changes Nov 4, 2024

View reviewed changes

wzieba added 6 commits November 4, 2024 16:22

Disable disabling UseTomlInstead in login module

20cd7bd

Merge branch 'trunk' into bump_android_lint

543395f

Use version catalog for dependencies that specify artifact type

32b261d

Related discussion: #21256 (comment)

Remove UseTomlInstead from disabled lint rules

d89ed5e

Remove IgnoreWithoutReason from disabled lint rules

94773c7

Suppress this warning on imported tests that are breaking this rule

Merge branch 'trunk' into bump_android_lint

ffb9840

wzieba enabled auto-merge November 7, 2024 11:55

wzieba merged commit 0fe0421 into trunk Nov 7, 2024
22 of 23 checks passed

wzieba deleted the bump_android_lint branch November 7, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wzieba commented Sep 24, 2024 •

edited

Loading

dangermattic commented Sep 24, 2024 •

edited

Loading

sonarqubecloud bot commented Sep 24, 2024

wpmobilebot commented Sep 24, 2024 •

edited

Loading

wpmobilebot commented Sep 24, 2024 •

edited

Loading

ParaskP7 Sep 25, 2024

wzieba Nov 1, 2024

ParaskP7 left a comment

ParaskP7 Nov 4, 2024

wzieba Nov 4, 2024

ParaskP7 Nov 4, 2024 •

edited

Loading

wzieba Nov 4, 2024

ParaskP7 Nov 4, 2024

wzieba Nov 7, 2024

sonarqubecloud bot commented Nov 7, 2024

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

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

Conversation

wzieba commented Sep 24, 2024 • edited Loading

Description

Testing

dangermattic commented Sep 24, 2024 • edited Loading

sonarqubecloud bot commented Sep 24, 2024

Quality Gate passed

wpmobilebot commented Sep 24, 2024 • edited Loading

wpmobilebot commented Sep 24, 2024 • edited Loading

ParaskP7 Sep 25, 2024

Choose a reason for hiding this comment

wzieba Nov 1, 2024

Choose a reason for hiding this comment

ParaskP7 left a comment

Choose a reason for hiding this comment

ParaskP7 Nov 4, 2024

Choose a reason for hiding this comment

wzieba Nov 4, 2024

Choose a reason for hiding this comment

ParaskP7 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

wzieba Nov 4, 2024

Choose a reason for hiding this comment

ParaskP7 Nov 4, 2024

Choose a reason for hiding this comment

wzieba Nov 7, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 7, 2024

Quality Gate passed

wzieba commented Sep 24, 2024 •

edited

Loading

dangermattic commented Sep 24, 2024 •

edited

Loading

wpmobilebot commented Sep 24, 2024 •

edited

Loading

wpmobilebot commented Sep 24, 2024 •

edited

Loading

ParaskP7 Nov 4, 2024 •

edited

Loading