Skip to content

[Compile Warnings As Errors] Processors Module - Resolve Warnings & Enable All Warnings as Errors#17197

Merged
ParaskP7 merged 6 commits intotrunkfrom
analysis/processors-all-warnings-as-errors
Sep 26, 2022
Merged

[Compile Warnings As Errors] Processors Module - Resolve Warnings & Enable All Warnings as Errors#17197
ParaskP7 merged 6 commits intotrunkfrom
analysis/processors-all-warnings-as-errors

Conversation

@ParaskP7
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 commented Sep 22, 2022 •
edited
Loading

Parent: #17173
Closes: #17175
Associated To: #17195

This PR resolves/suppresses a couple of warnings for the processors module and then enables all warnings as errors on it as well.

Warnings Resolution List:

  1. Analysis: Resolve append in deprecated warning.

Warnings Suppression List:

  1. Suppress kotlin stdlib's decapitalize deprecated warning.
  2. Suppress kotlin poet's as type name deprecated warning.

The 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 processors module, I took this opportunity to improve on other aspects of version 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: @irfano 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 processors module, along with the annotations 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.

@ParaskP7
Build: Move annotations/processors related version definitions to root
8106699
@ParaskP7
Build: Move editor related aztec version definition to root
cbef4e0 
This change also groups 'aztecVersion' to be part of the other library
related version definitions on root and has that sit closer to the
'gutenbergMobileVersion' version definition, which is also being
utilized mainly on the 'editor' module.
@ParaskP7
Analysis: Suppress kotlin stdlib's decapitalize deprecated warning
b8d4e45 
Warning Messages: "'decapitalize(Locale): String' is deprecated.
Use replaceFirstChar instead."

This deprecated warning is suppressed, that is, instead of it being
resolved, since using 'replaceFirstChar { ... }' is a bit complicated
and it would need a bit more thought.
@ParaskP7
Analysis: Resolve append in deprecated warning
31019f9 
Warning Messages: "'appendln(String?): kotlin.text.StringBuilder /* =
java.lang.StringBuilder */' is deprecated. Use appendLine instead. Note
that the new method always appends the line feed character '\n'
regardless of the system line separator."
@ParaskP7
Analysis: Suppress kotlin poet's as type name deprecated warning
476a7c4 
Warning Messages: "'asTypeName(): TypeName' is deprecated. Mirror APIs
don't give complete information on Kotlin types. Consider using the
kotlinpoet-metadata APIs instead."

This deprecated warning is suppressed, that is, instead of it being
resolved, since using the kotlinpoet-metadata APIs is out of scope.
@ParaskP7
Analysis: Enable all warnings as errors for the processors module
fab2ded
@ParaskP7 ParaskP7 added this to the 20.9 milestone Sep 22, 2022
@ParaskP7 ParaskP7 requested review from a team and irfano September 22, 2022 13:55
@ParaskP7 ParaskP7 self-assigned this Sep 22, 2022
This was referenced Sep 22, 2022
Merged
Closed
@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Sep 22, 2022

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

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Sep 22, 2022

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

irfano
irfano approved these changes Sep 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

I tested feature flags and they're still working.
The diff LGTM. 👍🏻 I'm not merging because others from apps-infrastructure may want to review too.
@ParaskP7, feel free to add me as the reviewer for future PRs.

@ParaskP7
Copy link
Copy Markdown
Contributor Author

ParaskP7 commented Sep 26, 2022

Thank you for reviewing and testing this @irfano ! 🙇

The diff LGTM. 👍🏻 I'm not merging because others from apps-infrastructure may want to review too.

Great, thanks for that! 🙏

FYI, next time please feel free to merge it yourself, don't worry about apps-infra team needing to review it too, I am just adding the team there:

  1. Mainly, so they could keep an eye on the overall progress of the project, and
  2. Secondarily, so they could also review the changes in addition to you review.

@ParaskP7, feel free to add me as the reviewer for future PRs.

Awesome, thank you so much for your willingness to help with the reviews Irfan, you rock! 🎸

@ParaskP7 ParaskP7 merged commit 3fec60c into trunk Sep 26, 2022
@ParaskP7 ParaskP7 deleted the analysis/processors-all-warnings-as-errors branch September 26, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@irfano irfano irfano approved these changes

Assignees

@ParaskP7 ParaskP7

Labels

Tech Debt Tooling

Projects

None yet

Milestone

20.9

Development

Successfully merging this pull request may close these issues.

Compiler Warnings as Errors - Processors Module

3 participants

@ParaskP7 @wpmobilebot @irfano