Conversation
Both these functions are unused and/or contains compile warnings that needs resolving. Warning Messages: - "Unsafe use of a nullable receiver of type Package?" - "Type mismatch: inferred type is String? but String was expected" Instead of resolving any compile warnings related to unused code, it is better to remove such code and stop maintaining it altogether. In the future, and if need be, this code can be added back, with an explicit comment that it shouldn't be removed anymore, thus suppressing it with an '@Suppress("unused")' annotation, along with a reasoning behind keeping unused code like that.
Warning Message: "Unsafe use of a nullable receiver of type Xyz?" After resolving some of the compile related type warnings, some Detekt baseline related warnings needed to be removed and suppressed closer to source because the lines on those classes got changed.
Warning Message: "Type mismatch: inferred type is Xyz? but Xyz was expected" After resolving some of the compile related type warnings, some Detekt baseline related warnings needed to be removed and suppressed closer to source because the lines on those classes got changed.
Warning Message: "Elvis operator (?:) always returns the left operand of non-nullable type Xyz"
Warning Message: "The corresponding parameter in the supertype 'Xyz' is named 'xyz'. This may cause problems when calling this function with named arguments."
Warning Message: "Unnecessary non-null assertion (!!) on a non-null receiver of type AppComponent"
Warning Message: "Unnecessary safe call on a non-null receiver of type Xyz. This expression will have nullable type in future releases"
Warning Message: "Unchecked cast: Xyz to Xyz" These type warnings are suppressed, that is, instead of being resolved, since a resolution would require a proper investigation and testing. As such, it might be best to ignore those as out of scope, for now, and so as to not introduce any breaking changes to these functionalities overall.
Warning Message: "This cast can never succeed" This type warning is suppressed, that is, instead of being resolved, since a resolution would require a proper investigation and testing. As such, it might be best to ignore this as out of scope, for now, and so as to not introduce any breaking changes to this functionality overall.
Warning Message: "Non exhaustive 'when' statements on enum will be prohibited in 1.7, add 'Xyz', 'Xyz' branch(es) or 'else' branch instead" You will notice that I optimized the imports on all those when statements and the imports they were using because one or no import is always better than having multiple imports for no particular reason. Also, having the extra information like 'FormattableRangeType.SITE' instead of plain 'SITE' is usually better for readability purposes as the reader will immediately understand that this 'SITE' is 'FormattableRangeType' related, without the need to navigate to the 'SITE' in order to understand when that is coming from.
This was referenced Oct 10, 2022
Contributor
|
|||||||||||
 |
💡 Scan this QR code with your Android phone to download and install the APK directly on it. | |
| App | Jetpack | |
| Build Flavor | Jalapeno | |
| Build Type | Debug | |
| Commit | f15bfc9 | |
Contributor
|
|||||||||||
 |
💡 Scan this QR code with your Android phone to download and install the APK directly on it. | |
| App | WordPress | |
| Build Flavor | Jalapeno | |
| Build Type | Debug | |
| Commit | f15bfc9 | |
ovitrif
approved these changes
Oct 11, 2022
Contributor
There was a problem hiding this comment.
The code changes look consistent to me and smoke testing both apps shows no regression ✅ .
Thank you for your effort on this project @ParaskP7 🙇 🥇 🎉
Contributor
Author
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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: #17173
Partially Closes: #17181
This PR resolves one part of all
Type/Cast/Whenrelated warnings for theWordPressmodule, and in this part, specific to the main source:Type Related (
44):4xUnsafe use of a nullable receiver of type Xyz?13xType mismatch: inferred type is Xyz? but Xyz was expected4xElvis operator (?:) always returns the left operand of non-nullable type Xyz4xThe corresponding parameter in the supertype 'Xyz' is named 'xyz'. This may cause problems when calling this function with named arguments.11xents. (4) Unnecessary non-null assertion (!!) on a non-null receiver of type Xyz18xUnnecessary safe call on a non-null receiver of type Xyz. This expression will have nullable type in future releasesCast Related (
5):4xUnchecked cast: Xyz to Xyz1xThis cast can never succeedWhen Related (
26):26xNon exhaustive 'when' statements on xyz will be prohibited in 1.7, add 'Xyz', 'Xyz' branch(es) or 'else' branch insteadWarnings Resolution List:
typewhenWarnings Suppression List:
castRefactor List:
typePS: @ovitrif 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:
WordPressandJetpackapps, and see if they are both working as expected.Regression Notes
N/A
See
To testsection above.N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.