[Compile Warnings As Errors] WordPress Module - Resolve Other Warnings by ParaskP7 · Pull Request #17293 · wordpress-mobile/WordPress-Android

ParaskP7 · 2022-10-10T14:46:00Z

Parent: #17173
Partially Closes: #17181

This PR resolves one part of all Other related warnings for the WordPress module, and in this part, (mainly) specific to the main source:

Main Source (35):

1 x This is a delicate API and its use requires care. Make sure you fully read and understand documentation of the declaration that is marked as a delicate API.
17 x Parameter 'xyz' is never used
9 x Parameter 'xyz' is never used, could be renamed to _
2 x Name shadowed: xyz
1 x Parcelize annotations from package 'kotlinx.android.parcel' are deprecated. Change package to 'kotlinx.parcelize'
1 x Variable 'xyz' initializer is redundant
1 x There is more than one label with such a name in this scope
2 x Condition 'xyz' is always 'xyz'
1 x Unreachable code

Test Source (5)

3 x Parameter 'xyz' is never used
2 x Variable 'xyz' is never used

Warnings Resolution List:

Warnings Suppression List:

Suppress parameter is never used warnings.

Refactor List:

Use suppress in favor of suppress warnings for kotlin classes.

PS: @zwarm I added you as the main reviewer, that is, in addition to @wordpress-mobile/apps-infrastructure team itself, but randomly, since I want someone from the WordPress Android team to primarily sign-off on that change. 🥇

FYI: I am going to randomly add more of you in those PRs that will follow, just so you become more aware of this change and how close we are on enabling allWarningsAsErrors by default everywhere. 🎉

To test:

There is nothing much to test here.
Verifying that all the CI checks are successful should be enough.
However, if you want to be thorough about reviewing this change, you could quickly smoke test both, the WordPress and Jetpack apps, and see if they are both working as expected.

Regression Notes

Potential unintended areas of impact

N/A

What I did to test those areas of impact (or what existing automated tests I relied on)

See To test section above.

What automated tests I added (or what prevented me from doing so)

N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Warning Message: "This is a delicate API and its use requires care. Make sure you fully read and understand documentation of the declaration that is marked as a delicate API." Adding the '@OptIn(DelicateCoroutinesApi::class)' annotation for the 'retrieveCapability(...)' function is the recommended action to resolve this 'GlobalScope' warning.

@Suppress

The Kotlin stdlib related '@Suppress' annotation is preferable to the Java lang related '@SuppressWarnings' annotation, if not for anything, even just for consistency purposes. As such, the '@SuppressWarnings' is being replaced with '@Suppress' on all Kotlin classes, wherever it is found.

Warning Message: "Parameter 'xyz' is never used" Notice that for 'Event Bus' related functions this warning should be suppressed instead. This is because this is how the 'Event bus' mechanism works as it need the parameter to be present on a function, even if that parameter is unused.

Warning Message: "Parameter 'xyz' is never used, could be renamed to _" Renaming the parameter 'xyz' that is never used to '_' or removing it completely is the recommended action to resolve this kind of warnings.

Warning Message: "Variable 'xyz' is never used"

Warning Message: "Name shadowed: xyz" Renaming the shadowed and/or the shadowing 'xyz' name is the recommended action to resolve this kind of deprecated warnings.

Warning Message: "Parcelize annotations from package 'kotlinx.android.parcel' are deprecated. Change package to 'kotlinx.parcelize'"

Warning Message: "Variable 'cursor' initializer is redundant"

Warning Message: "There is more than one label with such a name in this scope"

Warning Message: "Condition 'xyz' is always 'xyz'"

Warning Message: "Unreachable code"

wpmobilebot · 2022-10-10T15:01:48Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17293-3060469.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`3060469`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2022-10-10T15:02:57Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17293-3060469.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`3060469`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

zwarm

@ParaskP7 - I have taken the apps out for a spin. Nothing unusual, so 👍 . Thanks for including me a reviewer, it's been nice seeing the changes and helps me in the future. I left one question, non-PR related, so I'll be merging this PR.

🚢

zwarm · 2022-10-11T13:43:21Z

WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/utils/StatsUtils.kt

    ): List<String> {
        val contentDescriptions = mutableListOf<String>()
-        entries.forEachIndexed { index, bar ->
+        entries.forEachIndexed { _, bar ->


@ParaskP7 - all is good with the change you made ... just questioning why the use of forEachIndexed without actually using the index. 🤷 What am I missing? And I don't expect a change for this, it's not part of the PR.

Ha, yea, you are right @zwarm, this could have further improved to become a plain entries.forEach { bar -> ... }, the indexed version is not needed as the index ends up becoming unused. I am seeing this entries.forEachIndexed { index, bar -> ... } being actually used within getBarChartEntryContentDescriptions(...) above, and as such, I think this was just a copy-pasting overside at that point to create the getLineChartEntryContentDescriptions(...) below, which this time didn't have any need for that.

PS: I am going to fix that on my next PR. It would be just another refactoring commit on top of all the others commits! 🎉 Thanks for spotting it! 🙇 ❤️ 🌟

ParaskP7 · 2022-10-11T13:51:42Z

And again, thank you so much for reviewing, testing and merging this PR @zwarm, you are awesome! 🙇 ❤️ 🚀

ParaskP7 added 11 commits October 10, 2022 16:21

Analysis: Resolve parameter is never and could be renamed to _ warnings

528a991

Warning Message: "Parameter 'xyz' is never used, could be renamed to _" Renaming the parameter 'xyz' that is never used to '_' or removing it completely is the recommended action to resolve this kind of warnings.

Analysis: Resolve variable is never used test warnings

4a4f280

Warning Message: "Variable 'xyz' is never used"

Analysis: Resolve name shadowed warnings

4eee7ad

Warning Message: "Name shadowed: xyz" Renaming the shadowed and/or the shadowing 'xyz' name is the recommended action to resolve this kind of deprecated warnings.

Analysis: Resolve parcelize annotations deprecated package warning

53b180c

Warning Message: "Parcelize annotations from package 'kotlinx.android.parcel' are deprecated. Change package to 'kotlinx.parcelize'"

Analysis: Resolve variable initializer is redundant warning

bb6349c

Warning Message: "Variable 'cursor' initializer is redundant"

Analysis: Resolve more than one label with such a name in scope warning

32b8d59

Warning Message: "There is more than one label with such a name in this scope"

Analysis: Resolve condition is always true warnings

d6bd951

Warning Message: "Condition 'xyz' is always 'xyz'"

Analysis: Resolve unreachable code warning

3060469

Warning Message: "Unreachable code"

ParaskP7 added Tooling Tech Debt labels Oct 10, 2022

ParaskP7 added this to the 21.0 milestone Oct 10, 2022

ParaskP7 requested review from a team and zwarm October 10, 2022 14:46

ParaskP7 self-assigned this Oct 10, 2022

This was referenced Oct 10, 2022

Compiler Warnings as Errors - WordPress Module #17181

Closed

Compiler Warnings as Errors - Resolve/Suppress All Warnings #17173

Closed

zwarm approved these changes Oct 11, 2022

View reviewed changes

zwarm merged commit 09a3984 into trunk Oct 11, 2022

zwarm deleted the analysis/wordpress-main-other-warnings branch October 11, 2022 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Compile Warnings As Errors] WordPress Module - Resolve Other Warnings#17293

[Compile Warnings As Errors] WordPress Module - Resolve Other Warnings#17293
zwarm merged 11 commits intotrunkfrom
analysis/wordpress-main-other-warnings

ParaskP7 commented Oct 10, 2022

Uh oh!

wpmobilebot commented Oct 10, 2022

Uh oh!

wpmobilebot commented Oct 10, 2022

Uh oh!

zwarm left a comment

Uh oh!

zwarm Oct 11, 2022

Uh oh!

ParaskP7 Oct 11, 2022 •

edited

Loading

Uh oh!

ParaskP7 Oct 11, 2022

Uh oh!

ParaskP7 commented Oct 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ParaskP7 commented Oct 10, 2022

Regression Notes

Uh oh!

wpmobilebot commented Oct 10, 2022

Uh oh!

wpmobilebot commented Oct 10, 2022

Uh oh!

zwarm left a comment

Choose a reason for hiding this comment

Uh oh!

zwarm Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

ParaskP7 commented Oct 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ParaskP7 Oct 11, 2022 •

edited

Loading