[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Theme Network Client Classes (safe)
#2893
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Warning: "Link specified as plain text"
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
Warning: "Raw use of parameterized class 'Action'"
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
FYI: 'n-a' stands for 'nullability annotations'
Warning: "Field 'mThemeStore' may be 'final'"
Warning: "'application' is deprecated" Explanation: Accessing this field directly is inherently incompatible with 'org.robolectric.annotation.experimental.LazyApplication' and Robolectric makes no guarantees if a test modifies this field during execution. Replacing 'application' with 'getApplication()' resolves this deprecation warning.
Warning: "'assertTrue()' can be simplified to 'assertEquals()'"
Warnings: - "Test class name 'ReleaseStack_ThemeTestWPCom' doesn't match regex '[A-Z][A-Za-z\d]*Test(s|Case)?|Test[A-Z][A-Za-z\d]*|IT(.*)|(.*) IT(Case)?'" - "Test class name 'ReleaseStack_ThemeTestJetpack' doesn't match regex '[A-Z][A-Za-z\d]*Test(s|Case)?|Test[A-Z][A-Za-z\d]*|IT(.*)|(.*) IT(Case)?'"
Warning: "'assertTrue()' can be simplified to 'assertEquals()'"
Warning: "'assertTrue()' can be simplified to 'assertSame()'"
Warning: "'assertFalse()' can be simplified to 'assertNotEquals()'"
Warnings: - "Actual value of parameter 'username' is always 'BuildConfig.TEST_WPCOM_USERNAME_SINGLE_JETPACK_ONLY'" - "Actual value of parameter 'username' is always 'BuildConfig.TEST_WPCOM_USERNAME_SINGLE_JETPACK_ONLY'"
Warnings: - "Inner class 'WPComThemeListResponse' may be 'static'" - "Inner class 'WPComThemeMobileFriendlyTaxonomy' may be 'static'" - "Inner class 'WPComThemeTaxonomies' may be 'static'"
Warning: "Inner class 'JetpackThemeListResponse' may be 'static'"
A theme's id is not a number, at least not anymore. Instead it is a text like 'fewer' or 'mayland'. As such one cannot activate, install or delete a theme if that edit text is not updated to a 'text' input type.
FYI: 'n-a' stands for 'nullability annotations'. PS: This 'NotNullFieldNotInitialized' warning got suppressed because a 'response' class can never have its fields initialized via a constructor initialization, or otherwise for that matter.
FYI: 'n-a' stands for 'nullability annotations'. PS: This 'NotNullFieldNotInitialized' warning got suppressed because a 'response' class can never have its fields initialized via a constructor initialization, or otherwise for that matter. ------------------------------------------------------------------------ Also, you will note that the 'author', 'author_uri', 'theme_uri' and 'version' fields are being defined as non-null. If you compare them fields on the 'WordPress' equivalent response, you will notice them being nullable there. My testing revealed that this is indeed the case with Jetpack themes and thus those were marked as non-null here.
From the responses I tested, both, for WP.com and Jetpack, it seems that
'screenshot' is empty ("") instead of null, that is, when no screenshot
is set on a specific theme.
|
Thank you for tackling this @ParaskP7 🙇
I plan to go through this early next week.
Thank you for the clarification. I'll remove the |
Awesome, thank you for the heads-up on that @antonis , totally appreciated! 🙇 ❤️ |
16 tasks
…ndroid into analysis/add-missing-nullability-annotations-to-theme-client-classes
antonis
approved these changes
Nov 7, 2023
There was a problem hiding this comment.
Thank you for improving our codebase @ParaskP7 🙇
I went through the code and I don't have any comments on the code changes thanks to your detailed commit messages 🏅 I also smoke tested the WP/JP app and the example FluxC app and everything worked as expected for me on a Pixel 5 (Android 14) 🎉
antonis
deleted the
analysis/add-missing-nullability-annotations-to-theme-client-classes
branch
|
Awesome @antonis , thank you so much for reviewing, testing, and following the merge instructions, including merging this yourself, you rock! 🙇 ❤️ 🚀 |
This was referenced Nov 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Parent #2798
Accompanying JP/WPAndroid#19529
This PR adds any missing nullability annotation to ThemeRestClient.java, all its related response classes and anything in between (ie.
MediaStore). See response classes below:FYI.1: This change is
safe, meaning that there shouldn't be any (major) compile-time changes associated with this change. Having said that, testing is highly recommended since the changes in this PR can affect one or more clients (ie. JP/WPAndroid, WCAndroid, etc)FYI.2: An analysis on
ThemeStoreand its usages with our main clients suggests that this store is only being utilizing by JP/WPAndroid. AFAIA WCAndroid is not using that store and as such can be safely ignored from testing.PS.1: @antonis I added you as the main reviewer, randomly so, since I just wanted someone from the Jetpack/WordPress mobile team to be aware of and sign-off on that change. Feel free to merge this PR directly yourself if you deem so.
Nullability Annotation List (
Response):Nullability Annotation List (
Client):Nullability Annotation List (
Store):Nullability Checks List:
Warnings Resolutions List:
Warnings Suppression List:
Refactor List:
Test List:
Docs List:
Demo List:
Associated Clients
It is highly recommending that this change is being tested on the below associated clients:
To Test (
REST):FluxC Exampleapp via theTHEMEscreen. Verify everything is working as expected.To Test (
XMLRPC):N/A
1. [JP/WP] Themes Screen [ThemeBrowserActivity.java + ThemeBrowserFragment.kt]
ℹ️ This test applies to both, the
JetpackandWordPressapps.Themesscreen, verify it is shown and functioning as expected.Customize,DetailsandSupportbuttons. Verify that everything works as expected.Customize,DetailsandSupportbuttons. Verify that everything works as expected.DetailsandSupportbuttons. Verify that everything works as expected.ViewandTry & Customizebuttons. Verify that everything works as expected.Activatebutton. Verify that the selected theme is activated and that everything works as expected.2. [JP] Site Creation Screens [SiteCreationActivity.kt + HomePagePickerFragment.kt + DesignPreviewFragment.kt]
ℹ️ This test applies to the
Jetpackapp only.Choose Site, tap on the+button on top and via theCreate WordPress.com site, go toSite Creationscreen, verify it is shown and functioning as expected.What's your website about?screen, select a topic from the list of topics (ie.Food).Choose a themescreen, select a theme from the list of themes (per category, ieAbout).Previewscreen is shown and functioning as expected.Merge instructions:
[PR] Not Ready for Mergelabel.