Skip to content

[Compile Warnings As Errors] All Modules - Enable All Warnings as Errors#17318

Merged
ParaskP7 merged 5 commits intotrunkfrom
analysis/wordpress-all-warnings-as-errors
Oct 14, 2022
Merged

[Compile Warnings As Errors] All Modules - Enable All Warnings as Errors#17318
ParaskP7 merged 5 commits intotrunkfrom
analysis/wordpress-all-warnings-as-errors

Conversation

@ParaskP7
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 commented Oct 12, 2022 •
edited
Loading

Parent: #17173
Closes: #17181
Closes: #17182

This PR enables all warnings as errors for all modules.

This allWarningsAsErrors configuration was previously applied on the module level in order to make sure that, as the overall Kotlin Warnings as Errors work is progressing, no new warnings are added to a specific module, which is already free of warnings.

However, now that the overall Kotlin Warnings as Errors work is complete on all modules, this module level allWarningsAsErrors configuration is now replaced by a root level allWarningsAsErrors configuration that will be applied by default to all modules.

PS: @ovitrif 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. 🥇

To test:

  • There is nothing much to test here.
  • Verifying that all the CI checks are successful should be enough.
  • If you really want to be thorough, try to add an artificial warning per source (main, test, androidTest), per module (WordPress, editor, image-editor, processors, annotations) and see the assemble related task fail. Then, remove this warning and see it succeeding again.

Merge instructions

Regression Notes

  1. Potential unintended areas of impact

N/A

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

See To test section above.

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

@ParaskP7
Analysis: Enable all warnings as errors for the wordpress module
b9891c6
@ParaskP7
Analysis: Enable all warnings as errors for all modules
d9a7eef 
This 'allWarningsAsErrors' configuration was previously applied on the
module level in order to make sure that, as the overall 'Kotlin Warnings
as Errors' work is progressing, no new warnings are added to a
specific module, which is already free of warnings.

However, now that the overall 'Kotlin Warnings as Errors' work is
complete on all modules, this module level 'allWarningsAsErrors'
configuration is now replaced by a root level 'allWarningsAsErrors'
configuration that will be applied by default to all modules.
@ParaskP7 ParaskP7 added this to the 21.0 milestone Oct 12, 2022
@ParaskP7 ParaskP7 requested review from a team and ovitrif October 12, 2022 13:32
@ParaskP7 ParaskP7 self-assigned this Oct 12, 2022
@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Oct 12, 2022 •
edited
Loading

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17318-f7bc47e.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commitf7bc47e
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Oct 12, 2022 •
edited
Loading

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17318-f7bc47e.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commitf7bc47e
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

This was referenced Oct 12, 2022
Closed
Closed
Base automatically changed from analysis/wordpress-main-deprecated-warnings-part-2 to trunk October 13, 2022 11:58
@ParaskP7
Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-Android…
48c8e00 
… into analysis/wordpress-all-warnings-as-errors
ovitrif
ovitrif approved these changes Oct 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ovitrif ovitrif left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code changes lgtm 🎉

I've also tested by creating an artificial warning in a couple of sources and modules, resulting in failures of the related assemble task.

Thank you for the amazing effort to get this to the finish line @ParaskP7 🎉 🚀

P.S. I noticed that you've merged the PR branch with the latest trunk already; Please let me know if you want me to proceed with following the merging strategy 🙇

@ParaskP7
Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-Android…
2732e25 
… into analysis/wordpress-all-warnings-as-errors
@ParaskP7
Copy link
Copy Markdown
Contributor Author

ParaskP7 commented Oct 14, 2022

Thank you so much for reviewing this PR @ovitrif , we are at the end! 🙇 ❤️ 🚀

I've also tested by creating an artificial warning in a couple of sources and modules, resulting in failures of the related assemble task.

You are the best, thank you so much for going the extra mile with the testing part! 🌟 💯 🥇

Thank you for the amazing effort to get this to the finish line @ParaskP7 🎉 🚀

And thank YOU for holing my hand while doing so! 🎉 🚀

P.S. I noticed that you've merged the PR branch with the latest trunk already; Please let me know if you want me to proceed with following the merging strategy 🙇

Thanks for the notice, and double thanks for actually NOT merging this yet... 😅 I was waiting on @ajesh to land the Android 12 work first, and now that he did, as I updated this PR with trunk once more, I am ready to have it merged when CI gets to be successful! 🟢 😃 🏁

@ovitrif
Copy link
Copy Markdown
Contributor

ovitrif commented Oct 14, 2022

Thanks for the notice, and double thanks for actually NOT merging this yet... 😅 I was waiting on @ajesh to land the Android 12 work first, and now that he did, as I updated this PR with trunk once more, I am ready to have it merged when CI gets to be successful! 🟢 😃 🏁

Thank you for explaining Petros 🙇 🎉

Merged trunk again (to bypass installable-build failing 👀 p1665742041243539/1665733452.147889-slack-C5ALGJYEP)

Hopefully CI goes 🟢 now 🤞 🏁

@ParaskP7
Copy link
Copy Markdown
Contributor Author

ParaskP7 commented Oct 14, 2022

Merged trunk again (to bypass installable-build failing 👀 p1665742041243539/1665733452.147889-slack-C5ALGJYEP)

Thank you @ovitrif ! 🙇

Hopefully CI goes 🟢 now 🤞 🏁

🤞 Go 🟢 CI, go go go! 🏁

@ParaskP7 ParaskP7 merged commit aa00211 into trunk Oct 14, 2022
@ParaskP7 ParaskP7 deleted the analysis/wordpress-all-warnings-as-errors branch October 14, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ovitrif ovitrif ovitrif approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ParaskP7 ParaskP7

Labels

Tech Debt Tooling

Projects

None yet

Milestone

21.0

Development

Successfully merging this pull request may close these issues.

Compiler Warnings as Errors - All Modules - Enable All Warnings As Errors Flag Compiler Warnings as Errors - WordPress Module

3 participants

@ParaskP7 @wpmobilebot @ovitrif