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] Resolve Warnings & Enable All Warnings as Errors #1016

Merged
merged 17 commits into from
Nov 21, 2022

Conversation

ParaskP7
Copy link
Contributor

This PR resolves/suppresses all warnings for all modules and then enables all warnings as errors as the default for this project (a27076d).


Warnings Resolution List:

  1. Resolve kotlin stdlib's to lower case deprecated warnings
  2. Resolve parameter is never and could be renamed to _ warnings
  3. Resolve variable is never used warning
  4. Resolve type mismatch type warnings
  5. Resolve unsafe use of a nullable receiver type warning
  6. Resolve kotlin stdlib's sum by deprecated warnings
  7. Resolve name shadowed warning
  8. Resolve parameter supertype naming type warnings
  9. Resolve non exhaustive when statements when warning
  10. Resolve open has no effect in an object warnings
  11. Resolve type mismatch type warning for app module
  12. Resolve unnecessary safe call type warning for app module
  13. Resolve unnecessary safe call type warning for ui tests

Warnings Suppression List:

  1. Suppress get color deprecated warning
  2. Suppress variable initializer is redundant warning
  3. Suppress android junit4 deprecated warnings for ui tests

Test

  1. There is nothing much to test here.
  2. Verifying that all the CI checks are successful should be enough.
  3. However, if you want to be thorough about reviewing this change, you could quickly smoke test the example app and see if it is working as expected.

Review

@planarvoid and @danilo04 as you were also involved in #982.


Make sure strings will be translated:

  • If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.

Warning Message: "'toLowerCase(Locale): String' is deprecated.
Use lowercase() instead."

Replacing 'toLowerCase(...)' with 'lowercase(...)' is the recommended
action to resolve this kind of deprecated warnings.
Warning Message: "Parameter 'xyz' is never used, could be renamed to _"

Renaming the parameter 'xyz' that is never used to '_' or removing it
completely is the recommended action to resolve this kind of warnings.
Warning Message: "Parameter 'attrs' is never used"
Warning Message: "Type mismatch: inferred type is Xyz?
but Xyz was expected"
Warning Message: "Unsafe use of a nullable receiver of type Window?"
Warning Message: "'sumBy((T) -> Int): Int' is deprecated.
Use sumOf instead."

Replacing 'sumBy(...)' with 'sumOf(...)' is the recommended action to
resolve this kind of deprecated warnings.
Warning Message: "Name shadowed: start"
Warning Message: "The corresponding parameter in the supertype
'IAztecToolbar' is named 'keyEvent'. This may cause problems when
calling this function with named arguments."
Warning Message: "Non exhaustive 'when' statements on enum will be
prohibited in 1.7, add 'ADD_MEDIA_COLLAPSE', 'ADD_MEDIA_EXPAND',
'HIGHLIGHT', 'UNORDERED_LIST', 'ORDERED_LIST', 'TASK_LIST', 'INDENT',
... branches or 'else' branch instead"
Warning Message: "'getColor(Int): Int' is deprecated.
Deprecated in Java"

FYI: This is being suppressed instead of it being resolved since the
'minSdkVersion' is also about to be update to '24' and as such this
'Build.VERSION.SDK_INT < Build.VERSION_CODES.M' logic, along with
its deprecation, will get deleted altogether.
Warning Message: "'open' has no effect in an object"
Warning Message: "Variable 'obj' initializer is redundant"
Warning Message: "Type mismatch: inferred type is Bitmap? but Bitmap was
expected"
Warning Message: "Unnecessary safe call on a non-null receiver of type
Bundle. This expression will have nullable type in future releases"
Warning Message: "'AndroidJUnit4' is deprecated. Deprecated in Java"
Warning Message: "Unnecessary safe call on a non-null receiver of type
kotlin.collections.ArrayList<IAztecPlugin> /* =
java.util.ArrayList<IAztecPlugin> */. This expression will have nullable
type in future releases"
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

For full disclosure, there are a few cases where some code can be moved to the ?.let block, such as here or here, but I don't think they are particularly important - especially for this PR. So, I deliberately avoided a line comment, but then I felt uneasy that I am not mentioning it at all 😓


Related to null-ability changes; I wonder if there are cases where it'd be better to ensure that the value we are dealing is not null (by throwing an exception) rather than only doing the assignment if it's not null. However, I am not familiar enough with the codebase to tell where that'd make sense without investigating each change.

I suggest waiting for another approval from someone who has more experience with the codebase. It might also be a good idea to ask the editor to be beta tested after this is merged for the extra peace of mind.

@ParaskP7
Copy link
Contributor Author

👋 @oguzkocer !

Thank you so much for the review and testing, much appreciated! 🙇 ❤️ 🥇

For full disclosure, there are a few cases where some code can be moved to the ?.let block, such as here or here, but I don't think they are particularly important - especially for this PR. So, I deliberately avoided a line comment, but then I felt uneasy that I am not mentioning it at all 😓

True, I agree, especially on those two cases you mentioned. 💯

PS: I will go ahead and make the change, those seems easy enough to update anyway, thank you for spotting that! 🙇

Related to null-ability changes; I wonder if there are cases where it'd be better to ensure that the value we are dealing is not null (by throwing an exception) rather than only doing the assignment if it's not null. However, I am not familiar enough with the codebase to tell where that'd make sense without investigating each change.

Ha, this is a good point and I was thinking the same while applying the code changes myself, as I am not sure about the implication of suppressing the nullability by ignoring it like that vs. throwing an exception instead. The reason that I went ahead by applying changes that are suppressing the nullability is that in this #982 PR @planarvoid created a while back, he followed the same pattern. As such, on every change that I did, I went back to this PR and made sure that the same change were applied here too.

@planarvoid wdyt? 🤔

I suggest waiting for another approval from someone who has more experience with the codebase.

I agree, maybe @planarvoid can help with that extra approval too. 😊

It might also be a good idea to ask the editor to be beta tested after this is merged for the extra peace of mind.

I am shamelessly pinging @fluiddot here in case you can help with this, or at least so that you are aware what's coming in (also, the following PR should be the minSdkVersion = 24).

I'll also add it to my list of things to ask for an extra beta test of the editor on WPAndroid, after this PR is merged and when WPAndroid starts using that new version of Aztec that contain the changes.

That's all what you meant Oguz, right? 🤔

@fluiddot
Copy link
Contributor

It might also be a good idea to ask the editor to be beta tested after this is merged for the extra peace of mind.

I am shamelessly pinging @fluiddot here in case you can help with this, or at least so that you are aware what's coming in (also, the following PR should be the minSdkVersion = 24).

I'll also add it to my list of things to ask for an extra beta test of the editor on WPAndroid, after this PR is merged and when WPAndroid starts using that new version of Aztec that contain the changes.

Thanks for the ping @ParaskP7 🙇 ! I can perform a quick check in Gutenberg using this PR as a double-check, although I don't expect any issues based on the changes outlined in the PR's description 👍 .

@ParaskP7
Copy link
Contributor Author

Thanks for the ping @ParaskP7 🙇 ! I can perform a quick check in Gutenberg using this PR as a double-check.

That would be awesome, thank you so much for the extra hand @fluiddot ! 🙇 x 💯

...although I don't expect any issues based on the changes outlined in the PR's description 👍

Famous last words... 😅 🤞 🤞 🤞

@oguzkocer
Copy link
Contributor

That's all what you meant Oguz, right? 🤔

Yeap, that's all. Thank you @ParaskP7 🙇‍♂️

@ParaskP7
Copy link
Contributor Author

👋 @oguzkocer !

For full disclosure, there are a few cases where some code can be moved to the ?.let block, such as here or here, but I don't think they are particularly important - especially for this PR. So, I deliberately avoided a line comment, but then I felt uneasy that I am not mentioning it at all 😓

True, I agree, especially on those two cases you mentioned. 💯

PS: I will go ahead and make the change, those seems easy enough to update anyway, thank you for spotting that! 🙇

After looking at how to move some of the code to the ?.let { ... } block, I am now unsure about your suggestion as I can't think of a good way of moving this code and making it better. 🤔

Can you help me understand your suggestion, for example let's focus on this case here and see if we can improve it anyhow, thank you for your help! 🙏

@fluiddot
Copy link
Contributor

fluiddot commented Nov 18, 2022

Thanks for the ping @ParaskP7 🙇 ! I can perform a quick check in Gutenberg using this PR as a double-check, although I don't expect any issues based on the changes outlined in the PR's description 👍 .

I've checked this using #1017 and shared this comment. I still need further time to test this as we'd also need to bump the minSdkVersion to 24 in Gutenberg. As I discussed internally with @ParaskP7, I won't have the bandwidth to take a look until Nov 25th, 2022. I'll let you know when I have the results, thanks 🙇 !

@oguzkocer
Copy link
Contributor

Can you help me understand your suggestion, for example let's focus on this case here and see if we can improve it anyhow, thank you for your help! 🙏

@ParaskP7 As I mentioned in my original review, I don't think this is particularly important for this PR. I just mentioned it because I feel very uneasy when I see something and don't mention it at all. In any case, below is how I would have "improved" this function, but note that, although I believe this is more Kotlin-y, there is still some personal taste involved.

override fun onSaveInstanceState(): Parcelable? =
    super.onSaveInstanceState()?.let { superState ->
        SourceViewEditText.SavedState(superState).apply {
            state = Bundle().apply {
                putBoolean("isSourceVisible", sourceEditor?.visibility == View.VISIBLE)
                putBoolean("isMediaMode", isMediaModeEnabled)
                putBoolean("isExpanded", isExpanded)
                putBoolean("isMediaToolbarVisible", isMediaToolbarVisible)
                putByteArray(RETAINED_EDITOR_HTML_PARSED_SHA256_KEY, editorContentParsedSHA256LastSwitch)
                putByteArray(RETAINED_SOURCE_HTML_PARSED_SHA256_KEY, sourceContentParsedSHA256LastSwitch)
            }
        }
    }

I hope you find it useful. Let me know what you think!

P.S: Here is the documentation I consulted to double check that my suggestion is following Kotlin conventions. Hopefully I interpreted it correctly. 🤞

@ParaskP7
Copy link
Contributor Author

👋 @fluiddot !

I've checked this using #1017 and shared #1017 (review). I still need further time to test this as we'd also need to bump the minSdkVersion to 24 in Gutenberg. As I discussed internally with @ParaskP7, I won't have the bandwidth to take a look until Nov 25th, 2022. I'll let you know when I have the results, thanks 🙇 !

As per my response here, thank you so much for all your help, let's pick it up again after your return, this is not urgent work! 🙇 ❤️ 🙏

@ParaskP7
Copy link
Contributor Author

👋 @oguzkocer !

@ParaskP7 As I mentioned in my original review, I don't think this is particularly important for this PR. I just mentioned it because I feel very uneasy when I see something and don't mention it at all. In any case, below is how I would have "improved" this function, but note that, although I believe this is more Kotlin-y, there is still some personal taste involved.

Ah got it, thank you so much for sharing the code snippet, much appreciated, I now understand what you meant! 🙏

I was trying to figure out how to refactor by still keeping the number of nesting levels the same, naively thinking that this is what you meant, a quick win. But, I now see your recommendation more clearly. The only disadvantage of doing this refactor is the levels of nesting and the fact that using 3 chained scope functions like that might increase the mental power needed to understand what is going on. Thus, I might have stopped at the below alternative to keep the nesting at level 1, but as you said, with those refactoring, there is always personal taste involved.

    override fun onSaveInstanceState(): Parcelable? {
        val superState = super.onSaveInstanceState()
        val savedState = superState?.let { SourceViewEditText.SavedState(it) }
        savedState?.state = Bundle().apply {
            putBoolean("isSourceVisible", sourceEditor?.visibility == View.VISIBLE)
            putBoolean("isMediaMode", isMediaModeEnabled)
            putBoolean("isExpanded", isExpanded)
            putBoolean("isMediaToolbarVisible", isMediaToolbarVisible)
            putByteArray(RETAINED_EDITOR_HTML_PARSED_SHA256_KEY, editorContentParsedSHA256LastSwitch)
            putByteArray(RETAINED_SOURCE_HTML_PARSED_SHA256_KEY, sourceContentParsedSHA256LastSwitch)
        }
        return savedState
    }

No matter, thanks for bouncing ideas off that and how these functions can be further improved. I recommend keeping it as is for now, without introducing more changes, just because I think it is better to keep this PR connected to this #982 PR @planarvoid created, along with its changes, and chained with this upstream #1017 PR, without including unnecessary* extra changes in between.

(*) By unnecessary I don't mean that they are not valuable and welcome, I just mean that they won't do much to help with this PR's scope.

Copy link
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ParaskP7 ! I won't say I'm an expert on Aztec. I think I was trying to be overly cautious in my kotlin update PR. However, these changes are looking good! The only changes would be very nitpicky matter of taste so I'm fine with leaving this as is 👍

@planarvoid planarvoid merged commit 4358762 into trunk Nov 21, 2022
@planarvoid planarvoid deleted the analysis/all-warnings-as-errors branch November 21, 2022 13:01
@ParaskP7
Copy link
Contributor Author

Thank you so much for reviewing, testing and merging this @planarvoid , great stuff! 🚀 ❤️ 🥇

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.

4 participants