-
Notifications
You must be signed in to change notification settings - Fork 37
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
[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to TaxonomyRestClient
(safe
)
#2826
[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to TaxonomyRestClient
(safe
)
#2826
Conversation
Warning: - "Raw use of parameterized class 'WPComGsonRequest'"
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'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI.1: 'n-a' stands for 'nullability annotations'. FYI.2: 'trc' stands for 'taxonomy rest client'.
FYI.1: 'n-a' stands for 'nullability annotations'. FYI.2: 'trc' stands for 'taxonomy rest client'.
Warnings: - "Inner class 'TermsResponse' may be 'static'" - "Inner class 'Meta' may be 'static'" - "Inner class 'Links' may be 'static'"
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for well structured commits and helpful PR descriptions. LGTM! 👍🏻
I've also tested FluxC example app and JP app and couldn't discover any issue. 🟢
I've added two questions just for my curious, not a blocker but haven't merged the PR just in case. You can merge it after answering my questions.
...main/java/org/wordpress/android/fluxc/network/rest/wpcom/taxonomy/TermWPComRestResponse.java
Show resolved
Hide resolved
...main/java/org/wordpress/android/fluxc/network/rest/wpcom/taxonomy/TermWPComRestResponse.java
Show resolved
Hide resolved
Compile Warnings: - "Unnecessary safe call on a non-null receiver of type TermModel" - "Condition 'event.term.name == null' is always 'false'" ------------------------------------------------------------------------ As per the removed comment, and although it specifies that sometimes the API will return a success response with a null name, which then will be treated as an error, because without a name, there is no category, after some testing, it has been verified that this is not needed anymore. A '409 duplicate' error is returned instead, that is, instead of a valid response but with a 'null' name, which the deleted code was guarding against. This previous 'term.name == null' change is related to this JP/WPAndroid #13256 PR below: #13256 However, as per this newly merge FluxC #2826 PR below, the 'TermWPComRestResponse' response class and its 'name' field was marked as '@nonnull'. wordpress-mobile/WordPress-FluxC-Android#2826 PS: If and whenever the backend starts sending a 'term.name' as 'nullable', then this need to be addressed on the backend side, that is, instead of making 'term.name' a nullable field on the FluxC side and start guarding against such occurrences everywhere.
Parent #2798
This PR adds any missing nullability annotation to TaxonomyRestClient.java, all its related response classes and anything in between (ie.
TaxonomyStore
). 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
TaxonomyStore
and its usages with our main clients suggests that this store is only being utilizing JP/WPAndroid. AFAIA WCAndroid is not using that store and as such can be safely ignored from testing.PS: @irfano 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 for JP/WPAndroid. Feel free to merge this PR directly yourself if you deem so.
Nullability Annotation List:
Nullability Checks List:
Warnings Resolutions List:
Refactor List:
Associated Clients
It is highly recommending that this change is being tested on the below associated clients:
To Test:
FluxC Example
app via theTAXONOMIES
screen. Verify everything is working as expected.TaxonomyStore
related functionality on both, the WordPress and Jetpack apps, and see if everything is working as expected. For a couple of examples, you can expand and follow the inner and more explicit test steps within:1. Categories Settings Screen [CategoriesListFragment.kt]
ℹ️ This test applies to both, the
Jetpack
andWordPress
app.Menu
sub-tab on the initialMy Site
screen and click onSite Settings
under theManage
section.Categories
under theWriting
section and click on it.Categories
screen is displayed and that everything works as expected.2. Tags Settings Screen [SiteSettingsTagListActivity.kt]
ℹ️ This test applies to both, the
Jetpack
andWordPress
app.Menu
sub-tab on the initialMy Site
screen and click onSite Settings
under theManage
section.Tags
under theWriting
section and click on it.Tags
screen is displayed and that everything works as expected.