Skip to content

[Compile Warnings As Errors] Image Editor Module - Resolve Warnings & Enable All Warnings as Errors#17201

Merged
zwarm merged 3 commits intotrunkfrom
analysis/image-editor-all-warnings-as-errors
Oct 11, 2022
Merged

[Compile Warnings As Errors] Image Editor Module - Resolve Warnings & Enable All Warnings as Errors#17201
zwarm merged 3 commits intotrunkfrom
analysis/image-editor-all-warnings-as-errors

Conversation

@ParaskP7
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 commented Sep 23, 2022 •
edited
Loading

Parent: #17173
Closes: #17179

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

Warnings Resolution List:

  1. Resolve to lower case deprecated warning.

Warnings Suppression List:

  1. Suppress bitmap's compress format webp 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).

PS: @ravishanker @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 test any image editor related functionality, since this image-editor module is responsible for that. For example, you could try cropping an image on a post and see if that works as expected.

Regression Notes

  1. Potential unintended areas of impact

The image editor 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
Analysis: Suppress bitmap's compress format webp deprecated warning
6b733bc 
Warning Messages: "'WEBP' is deprecated. Deprecated in Java"

This deprecated warning is suppressed, that is, instead of it being
resolved, since using the more explicit 'WEBP_LOSSY' and 'WEBP_LOSSLESS'
is a bit complicated and it would need a bit more thought.
@ParaskP7
Analysis: Resolve to lower case deprecated warning
ed8ae3d 
Warning Message: "'toLowerCase(Locale): String' is deprecated.
Use lowercase() instead."
@ParaskP7
Analysis: Enable all warnings as errors for the image editor module
30a06c2
@ParaskP7 ParaskP7 added this to the 20.9 milestone Sep 23, 2022
@ParaskP7 ParaskP7 requested review from a team and ravishanker September 23, 2022 11:36
@ParaskP7 ParaskP7 self-assigned this Sep 23, 2022
@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Sep 23, 2022

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

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Sep 23, 2022

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

@ParaskP7 ParaskP7 changed the title [Compile Warnings As Errors] Image Editor Module - Resolve Warnings & Enable All Warnings as Errors #17197 [Compile Warnings As Errors] Image Editor Module - Resolve Warnings & Enable All Warnings as Errors Sep 23, 2022
@ParaskP7 ParaskP7 mentioned this pull request Sep 23, 2022
Closed
2 tasks
@AliSoftware AliSoftware modified the milestones: 20.9, 21.0 Oct 3, 2022
@ParaskP7
Copy link
Copy Markdown
Contributor Author

ParaskP7 commented Oct 4, 2022

👋 @ravishanker !

Apologies for the direct ping, but I wonder if you have the capacity for reviewing and testing this PR. Let me know, and if not, I can assign it to another engineer. I would like to avoid keeping this PR open for much longer, risking it becoming stale.

@ParaskP7
Copy link
Copy Markdown
Contributor Author

ParaskP7 commented Oct 10, 2022

👋 @ravishanker !

As I am seeing you being busy with other priorities I am assigning this PR review to @zwarm for now. Annmarie, let me know if that's okay with you too. I have updated the PR description accordingly. Thank you all for helping out in reviewing, testings and ultimately merging this to trunk. 🙇

@ParaskP7 ParaskP7 requested review from zwarm and removed request for ravishanker October 10, 2022 08:51
@zwarm
Copy link
Copy Markdown
Contributor

zwarm commented Oct 10, 2022

@ParaskP7 - Can the review hold for a day or so? 👍 I won't have time to get to it today.

@ParaskP7
Copy link
Copy Markdown
Contributor Author

ParaskP7 commented Oct 10, 2022

@ParaskP7 - Can the review hold for a day or so? 👍 I won't have time to get to it today.

👋 @zwarm and thank you so much, short answer yes, please take your time with it! 🙇

PS: This PR is not that urgent, so please feel free to review it when you get some extra time this week. It is just that this needs to be done sooner than later in order to avoid having it become stale.

zwarm
zwarm approved these changes Oct 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

@ParaskP7 🥇
All looks good. I also took media out for a spin and didn't experience any wonkiness while editing images.

Thanks!

@zwarm zwarm merged commit e2eb39b into trunk Oct 11, 2022
@zwarm zwarm deleted the analysis/image-editor-all-warnings-as-errors branch October 11, 2022 13:31
@ParaskP7
Copy link
Copy Markdown
Contributor Author

ParaskP7 commented Oct 11, 2022 •
edited
Loading

Awesome, thank you for reviewing, testing and merging this PR @zwarm, you rock! 🙇 ❤️ 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zwarm zwarm zwarm approved these changes

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 - Image Editor Module

4 participants

@ParaskP7 @wpmobilebot @zwarm @AliSoftware