Skip to content

[Compile Warnings As Errors] Annotations Module - Enable All Warnings as Errors#17195

Merged
ParaskP7 merged 9 commits intotrunkfrom
analysis/annotations-all-warnings-as-errors
Sep 23, 2022
Merged

[Compile Warnings As Errors] Annotations Module - Enable All Warnings as Errors#17195
ParaskP7 merged 9 commits intotrunkfrom
analysis/annotations-all-warnings-as-errors

Conversation

@ParaskP7
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 commented Sep 22, 2022

Parent: #17173
Closes: #17174
Associated To: #17197

This PR enables all warnings as errors for the annotations module.

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

When the overall Compiler Warnings as Errors work is complete on all modules, then this module level allWarningsAsErrors configuration will be replaced by a root level such configuration that will be applied by default to all modules (see here).


Note that, in addition to enabling the allWarningsAsErrors configuration on the annotations module, I took this opportunity to improve on other aspects of java and build related configurations. As such, this PR also improves on the following in order to mainstream this (kind of related) group of configurations, for this and all other lib module:


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

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 test the feature flag functionality, since this annotations module, along with the processors module is responsible for that. For example, you could try switching the jetpack_powered_bottom_sheet_remote_field feature flag, on and off, and see if that works as expected.

Regression Notes

  1. Potential unintended areas of impact

The feature flag functionality is not workings as expected.

  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.

This 'compileTestKotlin' configuration block seems unnecessary. The
corresponding 'compileKotlin' configuration block should be all that is
needed in that case.

Also, there is no 'test' source associated with this module, and as such
this makes the 'compileTestKotlin' configuration block even more
unnecessary.
This configuration was removed from both, the 'annotations' and
'processors' modules.

This configuration was removed as a follow-up on this discussion below
when its was discussed whether we should be removing the JVM
source/target explicitly at that point. After a short discussion, it
was decided to not do that back then, but nowadays there is no much
point in keeping this configuration any longer.

Discussion: wordpress-mobile/WordPress-iOS#19335
There is no other such 'compileOptions' configuration on any other
'libs' related module, only on the 'apps' related 'WordPress' module.

As such, this configuration on the 'editor' related libs module seems
redundant and thus removed.
This configuration already exist on the root level 'build.gradle'
configuration file, which applies it by default to all modules within
the 'tasks.withType(KotlinCompile).all { ... }' block.
@ParaskP7 ParaskP7 added this to the 20.9 milestone Sep 22, 2022
@ParaskP7 ParaskP7 requested a review from ovitrif September 22, 2022 11:35
@ParaskP7 ParaskP7 self-assigned this Sep 22, 2022
@wpmobilebot
Copy link
Copy Markdown
Contributor

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr17195-b7704dd.apk), or scanning this QR code:

@wpmobilebot
Copy link
Copy Markdown
Contributor

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr17195-b7704dd.apk), or scanning this QR code:

Copy link
Copy Markdown
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

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

Great work again @ParaskP7 🥇 I love the improvements too!

Thanks for offering me the honour to review this 😊

I tested by toggling a feature flag and it works as expected 🚀 , the CI checks are also green 🎉

:shipit:

@ParaskP7
Copy link
Copy Markdown
Contributor Author

Thanks for offering me the honour to review this 😊

The honour is all mine @ovitrif , thanks for the review and testing, you rock! 🌟

@ParaskP7 ParaskP7 merged commit b5c298c into trunk Sep 23, 2022
@ParaskP7 ParaskP7 deleted the analysis/annotations-all-warnings-as-errors branch September 23, 2022 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compiler Warnings as Errors - Annotations Module

3 participants