-
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 All Comment
SqlUtils & Model Classes (breaking
)
#2870
Merged
ParaskP7
merged 25 commits into
trunk
from
analysis/add-missing-nullability-annotations-to-all-comment-sqlutils-model-classes
Oct 19, 2023
Merged
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
This transient 'level' field doesn't seem to be used within FluxC or any client app that might be using the this 'CommentModel' model. FYI: There is another such definition of a transient 'level' field, which is actually still in use, but that is on the JP/WPAndroid level and via the 'ReaderComment' model. PS: Actually, JP/WPAndroid is the only client that is using this 'CommentModel' model, or 'CommentStore' for that matter.
FYI: 'n-a' stands for 'nullability annotations'. PS: This 'NotNullFieldNotInitialized' warning got suppressed because a table related 'model' class can never have its fields initialized via a constructor initialization, or otherwise for that matter.
FYI: 'n-a' stands for 'nullability annotations'.
Warning: "'switch' statement on enum type 'org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType' misses cases: 'TIMEOUT', 'NO_CONNECTION', 'NETWORK_ERROR', 'NOT_FOUND', 'CENSORED', 'SERVER_ERROR', 'INVALID_SSL_CERTIFICATE', 'HTTP_AUTH_ERROR', 'AUTHORIZATION_REQUIRED', 'NOT_AUTHENTICATED', 'PARSE_ERROR' and 'UNKNOWN'"
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: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
Warning: "Collections.sort could be replaced with List.sort"
Warning: "Expression can be replaced with 'Long.compare'"
Warning: "'SQLiteDatabase' used without 'try'-with-resources statement"
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'.
16 tasks
Base automatically changed from
analysis/add-missing-nullability-annotations-to-comment-clients
to
trunk
October 18, 2023 11:37
…ndroid into analysis/add-missing-nullability-annotations-to-all-comment-sqlutils-model-classes
wzieba
approved these changes
Oct 18, 2023
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.
Solid work @ParaskP7 ! Pre-approving as I haven't found dangerous blockers. Leaving some comments for discussion though.
fluxc/src/main/java/org/wordpress/android/fluxc/utils/CommentErrorUtils.java
Outdated
Show resolved
Hide resolved
fluxc/src/main/java/org/wordpress/android/fluxc/persistence/CommentSqlUtils.java
Show resolved
Hide resolved
fluxc/src/main/java/org/wordpress/android/fluxc/persistence/CommentSqlUtils.java
Show resolved
Hide resolved
Related PR Comment: https://github.com/wordpress-mobile/ WordPress-FluxC-Android/pull/2870#discussion_r1363993819
Thanks for answering comments and the commit, LGTM ! @ParaskP7 |
ParaskP7
deleted the
analysis/add-missing-nullability-annotations-to-all-comment-sqlutils-model-classes
branch
October 19, 2023 09:44
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 #2799
Closes #2831
This PR adds any missing nullability annotation to all
Comment
related sql-util & model classes.FYI.1: This change is
breaking
, meaning that any client that depended on theComment
model should update to the new APIs. As such, this change is inherentlyrisk
, meaning that there are compile-time changes associated with this change and thus needs testing to verify correctness, both on the library's and client's side.FYI.2: Comparing to a previous such PR, in this PR there are no
non-default constructors
anddefault constructor
changes. This is because of the fact that there are a couple of pseudoComment
model construction usages within the JP/WPAndroid repo (here and here), which makes it really hard to re-define and scope theComment
model. As such, and in order to avoid causing a larger refactor, addingnon-default constructors
constructors and deprecating thedefault constructor
onComment
model was ignored.PS.1: Any other changes on any other class are just some propagating missing nullability annotation changes, which stem from the main set of classes being updated, this PR's focus, plus, some other extra minor analysis, refactor and test related changes.
PS.2: As part of this 3821422 commit, the CommentsMapper.kt and its commentEntityToLegacyModel(...) function got updated to respect the updated nullability contract of the CommentModel.java model class, which is no longer allowing for a nullable
mStatus
,mDatePublished
,mContent
andmUrl
fields. From my understanding, the author of CommentsMapper.kt created those mappings and the CommentEntity database entity by defaulting all non primitive fields of theComment
model to being nullable. But, from my testing, and this related PR (its response) at least 4 of them can't and shouldn't be nullable, as in some cases, if those were to be defined as nullable, that would have caused a NPE. Be extra mindful of that when reviewing this PR.@wzieba let me know how I did on this PR, that is, in terms of updating the store/sqlutils classes and adding nullability annotations on them and their main CommentModel.java model class, in order to utilize the nullability annotations added on its fields and thus make its use null proof. If you also feel that this is a good way to proceed with such PRs, I'll then use this pattern across the board and follow a similar approach on every such PR.
Nullability Annotations List (
Model
):Nullability Annotations List (
SqlUtils
):Nullability Annotations List (
Store
):Nullability Annotations List (
Utils
):Nullability Checks List:
Warnings Resolution List:
Warnings Suppression List:
Refactor List:
Associated Clients
It is highly recommending that this change is being tested alongside the below associated clients and their accompanying PRs:
To Test (
REST
):FluxC Example
app via theCOMMENTS
screen. Verify everything is working as expected.CommentStore
andCommentsStore
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:To Test (
XMLRPC
):FluxC Example
app via theCOMMENTS
screen. Verify everything is working as expected.CommentStore
andCommentsStore
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. Comment Details Screen from Comments Screen [CommentDetailFragment.java]
ℹ️ This test applies to both, the
Jetpack
andWordPress
app.Comments
screen and tap on any comment.Comment Details
screen is shown and functioning as expected.2. Comment Details Screen from Notification Tab [CommentDetailFragment.java]
ℹ️ This test applies to the
Jetpack
app only.Notifications
tab, then itsCOMMENTS
sub-tab, and tap on any comment.Comment Details
screen is shown and functioning as expected.3. Edit Comment Screen from Comments Screen [UnifiedCommentsEditActivity.java]
ℹ️ This test applies to both, the
Jetpack
andWordPress
app.Comments
screen and tap on any comment.More
on the right side, at the bottom of the comment, selectEdit
.Edit Comment
screen is shown and functioning as expected.Name
,Web address
,Email address
and itsComment
itself.4. Edit Comment Screen from Notification Tab [UnifiedCommentsEditActivity.java]
ℹ️ This test applies to the
Jetpack
app only.Notifications
tab, then itsCOMMENTS
sub-tab, and tap on any comment.More
on the right side, at the bottom of the comment, selectEdit
.Edit Comment
screen is shown and functioning as expected.Name
,Web address
,Email address
and itsComment
itself.5. Reader Comments Screen [ReaderCommentListActivity.java]
ℹ️ This test applies to the
Jetpack
app only.Reader
tab, then itsDISCOVER
sub-tab, and tap on any post.Comments
button at the bottom of the post.Reader Comments
screen is shown and functioning as expected.Merge instructions:
[PR] Not Ready for Merge
label.