Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Sep 23, 2022

Parent: #17173
Closes: #17180

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

Warnings Resolution List:

  1. Resolve unnecessary safe call type 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.1: @SiobhyB 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. 🥇
PS.2 @SiobhyB I actually didn't add you here too randomly as I want you to verify this e053210 change I did, which reverts your such 24a0532 change (as part of this PR). I am actually not sure why you did this change, thus please do let me know if my change isn't breaking anything that you tried to fix with your 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 editor related functionality, since this editor module is responsible for that. For example, you could try replacing the featured image on a post, from a post image (not the settings page), and see if that works as expected.

Regression Notes

  1. Potential unintended areas of impact

The editor functionality is not working 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.

Warning Message: "Unnecessary safe call on a non-null receiver of type
CharSequence. This expression will have nullable type in future
releases"

The 'mPositiveButtonLabel' is being reverted back to being a nullable
field, that is, instead of it being a 'lateinit' non-null field.

PS: This field was nullable in the past, but this
24a0532 commit made it into
'lateinit' non-null.
@ParaskP7 ParaskP7 added this to the 20.9 milestone Sep 23, 2022
@ParaskP7 ParaskP7 requested review from SiobhyB and a team September 23, 2022 13:27
@ParaskP7 ParaskP7 self-assigned this Sep 23, 2022
@wpmobilebot
Copy link
Contributor

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

@wpmobilebot
Copy link
Contributor

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

Copy link
Contributor

@SiobhyB SiobhyB left a comment

Choose a reason for hiding this comment

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

PS.2 @SiobhyB I actually didn't add you here too randomly as I want you to verify this e053210 change I did, which reverts your such 24a0532 change (as part of #14503 PR). I am actually not sure why you did this change, thus please do let me know if my change isn't breaking anything that you tried to fix with your change.

Thank you for the ping, @ParaskP7! I looked over the original commit and PR, but can't recall a specific reason for the change. It was from the very early days of my apprenticeship, so it's likely to have been an amateur response to something not working as expected. 🤔 A good reminder to myself to actually write useful commit messages, too!

I downloaded the installable build and confirm the dialog still displays as expected, with no visual errors, when changing between featured images within the editor. Thank you for cleaning this up!

@ParaskP7
Copy link
Contributor Author

👋 @SiobhyB and thank you for reviewing and testing this PR! 🥇

Thank you for the ping, @ParaskP7! I looked over the original commit and PR, but can't recall a specific reason for the change.

Awesome, thank you so much for confirming that for me, you rock! 🙇

@ParaskP7 ParaskP7 merged commit 9b62703 into trunk Sep 23, 2022
@ParaskP7 ParaskP7 deleted the analysis/editor-all-warnings-as-errors branch September 23, 2022 15:24
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 - Editor Module
3 participants