Compiler Warnings as Errors - WordPress Module - AsyncTask

ParaskP7 commented 2 years ago

Parent #17173

This issue is about resolving the AsyncTask compile warnings for the WordPress module, and possibly, for the lib modules as well.

PS: Some of those warnings are already deprecated (see here).

This AsyncTask class was deprecated in API level 30. Use the standard java.util.concurrent or Kotlin concurrency utilities instead.

For more info see:

AsyncTask

peril-wordpress-mobile[bot] commented 2 years ago

	Fails
:no_entry_sign:	Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by :no_entry_sign: dangerJS

peril-wordpress-mobile[bot] commented 2 years ago

	Fails
:no_entry_sign:	Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by :no_entry_sign: dangerJS

peril-wordpress-mobile[bot] commented 2 years ago

	Fails
:no_entry_sign:	Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by :no_entry_sign: dangerJS

peril-wordpress-mobile[bot] commented 2 years ago

	Fails
:no_entry_sign:	Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by :no_entry_sign: dangerJS

shifli7 commented 1 year ago

Hi @ParaskP7 👋 I would like take this Task. Is it available?

ParaskP7 commented 1 year ago

FYI: I navigated to https://github.com/Shifli-N but I get a 404. It looks like the Shifli-N user is no longer valid. @Shifli-N, please do correct me if I am wrong or if you've changed your username.

shifli7 commented 1 year ago

I changed username while ago. Pls Check here, https://github.com/shifli7

ParaskP7 commented 1 year ago

👋 @shifli7 , that was weird, not sure what happened there with GitHub as I am now seeing both your comments (here and here) with your correct new username! 🤷 😅

Let's continue then... But first, thank you so much for offering to contribute! 🥇

Please feel free to start working on this. As such, I'll go ahead and assign this task to you (and me). PS: I am assigning myself as well, as I'll be guiding you all the way, up until we collaboratively merge your solution to trunk.

Before you begin, make sure to at least read the following documentation:

README.md
- Build Instructions (Step.2): You might notice that you would need to install npm. This is done for block editor related purposes (see gutenberg and gutenberg-mobile). The block editor is all React Native and as such this is a requirement for when working on it, especially for testing purposes. However, feel free to skip this step, that is, until you need it. PS: Most probably you won't be needing it for this task.
CONTRIBUTING.md
docs/coding-style.md

FYI: Note that depending on your solution, number of PRs, PR description, commit strategy, etc, I might ask you to improve on the quality and sometimes, even redo a PR. But, don't be afraid of it, I will only do that to help you out. Since this will be one of your first contribution to this repo, it is all part of the learning process, that is, until you become comfortable and aware about the ins-and-outs of how we work.

Do let me know if you need anything else! 🌟

shifli7 commented 1 year ago

Hey @ParaskP7, Thank you for assigning. I will start to work on it. Sure if I have any queries I'll reach you out.

shifli7 commented 1 year ago

Hello @ParaskP7

PR - https://github.com/wordpress-mobile/WordPress-Android/pull/18139

Above In PR, I made a base functionality as like AsyncTask using Kotlin Coroutines and pushed initial commit. Can you take a look on this and give so feedback to Improve and Cleanup the code.

ParaskP7 commented 1 year ago

👋 @shifli7 and thank you so much for starting you work on that! 👍

I have just quickly reviewed this PR you've opened but I would like us to step back for a second and see what's the best way forward, let me explain:

First, it might be better for us to discuss the solution here prior to you doing any kind of work. Think of it as the planning phase.
Then, after us making our mind on how to best approach this work, we could start laying down a concrete plan.
Finally, you (we) can start making process towards that plan, one small step at a time.

Does the above make sense to you? 🤔

Without the above in place, we end-up with situations like this PR of yours where you will spend a significant amount of time and then not being able to move forward anyway. Let me explain:

You created a PR where you want to replace android.os.AsyncTask with your own version of org.wordpress.android.util.KotlinAsyncTask, which is just going to act as a wrapper to help remove the deprecation on AsyncTask. Now, I do understand why you did that, because in this way, you envisioned fixing this deprecation for all screens/functionalities at once. Given the fact that most of them as still written in Java, this was the only quick way.
However, (IMHO) this might not be the best way forward and I wouldn't be comfortable for us to switch from AsyncTask and into our own KotlinAsyncTask class, then maintaining it as well. But worse than that, the fundamental problem will remain, which is, the fact that using this AsyncTask <Params, Progress, Result> pattern is too complicated, might cause context leaks, missed callbacks, or crashes amongst other issues.
Instead, what I had in mind was going towards the use of Kotlin coroutines with lifecycle-aware components explicitly, just like you would anyway do nowadays, that is, if you were to build a screen from scratch. Obviously, to do so, one would first need to convert these classes to Kotlin. Thus, my preferred plan would have been the following:
1. Per class, first convert it from Java to Kotlin, then test it to make sure that everything is working as expected, providing explicit testing instructions, finally create a PR.
2. Then, per class, convert the usage of AsyncTask into Kotlin Coroutines, then test it to make sure that everything is working as expected, providing explicit testing instructions, finally create a PR. FYI: As a reference and an example, a colleague of ours did something similar here (dfefdab05c864cfedbc364d2cafde03916e8901c).

As you understand, with the above, there is no silver pullet to fix and resolve this AsyncTask deprecation on every occurrence and all at once. Instead, I propose to take it slow and do that progressively, one Java-to-Kotlin conversion PR at a time, one AsyncTask-to-Coroutines migration at a time. I counted 13 files in total, files that still utilize android.os.AsyncTask, which means that we are looking at 13x2=26 PRs in total.

How does that sound to you? 🤔

shifli7 commented 1 year ago

Hi @ParaskP7,

Sorry for raising PR without even basic discussion. This my first public repo contribution. Now I got it. Sounds Great 👍, Sure we will go with the way you suggested. I think that is good way to approach. I'll start working on the way you suggested. I'll take reference as this one (https://github.com/wordpress-mobile/WordPress-Android/commit/dfefdab05c864cfedbc364d2cafde03916e8901c). I'll revert my changes and I delete my PR(https://github.com/wordpress-mobile/WordPress-Android/pull/18139)

ParaskP7 commented 1 year ago

👋 @shifli7 !

Sorry for raising PR without even basic discussion. This my first public repo contribution. Now I got it.

No worries, that's why we are here, to support you towards your first public contribution, one small step at a time, let's do this! ❤️ 🚀

Sounds Great 👍, Sure we will go with the way you suggested. I think that is good way to approach. I'll start working on the way you suggested.

Awesome, let's start small with a Java-to-Kotlin conversion PR first, on a specific class (maybe the CommentsDetailsActivity.java class), test that thoroughly and then take it from there with yet another PR that will handle the AsyncTask-to-Coroutines migration, for that specific class, while testing it thoroughly again and verify that everything is looking and working as expected.

PS: I suggest you first get acquainted with the class you will target, test the screen associated with it and verify that you are aware of all its functionalities, that is, before progressing with any change (Java-to-Kotlin or AsyncTask-to-Coroutines). This way, after you go ahead with any change, during testing, you (and us) will be fully comfortable that everything is still working as expected and you will be able to provide adequate testing instructions for the reviewer. Let me know if that makes sense to you too. 🙏

I'll take reference as this one (https://github.com/wordpress-mobile/WordPress-Android/commit/dfefdab05c864cfedbc364d2cafde03916e8901c). I'll revert my changes and I delete my PR(https://github.com/wordpress-mobile/WordPress-Android/pull/18139)

👍 🍀 👍

shifli7 commented 1 year ago

Hello @ParaskP7,

PR commit: CommentsDetailsActivity.kt

I have converted the CommentsDetailsActivity.java in Java-to-Kotlin. Also I checked is working good.

I faced build issue, while running the application after conversion this file to Kotlin. Because issue not able to run. Issue is raised due the warnings in Kotlin file. So I checked and i made temporary change on this flag allWarningsAsErrors = true to false in build.gradle. Now I'm able to build and run the project.

Is there any other way to fix this ?

ParaskP7 commented 1 year ago

👋 @shifli7 and thanks on starting work on that, this is awesome!

I have converted the CommentsDetailsActivity.java in Java-to-Kotlin. Also I checked is working good.

First, let me share with you a nice little trick when converting a file from Java-to-Kotlin. Before committing, make sure to check the Extra commit for .java > .kt renames option. This will make sure to do a little dance when the file will be converted in a two-step process:

The first commit will end up being just a rename: Rename .java to .kt
The second commit will rename back the .kt extension to .java and do the actual commit: ie. Kotlin: Kotlinize comments detail activity

Now, you might ask why this is important. Well, it is important because doing so, one will be able to do a diff on this second commit and see exactly what has changes, between the Java and the Kotlin file. Otherwise, you would just get a file deletion (the .java file) and a file addition (the .kt file), which will make it difficult for you, or anyone else to diff, that is, unless you copy the previous Java file and compare it to the newly added Kotlin file.

FYI: Doing so helps very much during the code review process.

Also, I would recommend that when you are doing such a conversion, to solely depend on the automatic conversion and commit it straight away, without making any refinements to the resulting Kotlin file, even if it doesn't compile. Then, on a subsequent commit, you can do any refinements, like making some code look more idiomatic. This will help inform the reviewer and anyone else looking at it in the future understand that no human touch was required during the first step, the automatic conversion.

Does the above make sense or help you, just this bit? 😊

I faced build issue, while running the application after conversion this file to Kotlin. Because issue not able to run. Issue is raised due the warnings in Kotlin file. So I checked and i made temporary change on this flag allWarningsAsErrors = true to false in build.gradle. Now I'm able to build and run the project.

You are right, that you now must deal with warnings being treated as errors on this newly converted Kotlin file. This is because of the allWarningsAsErrors flag, which is KotlinCompile related and applies to Kotlin files only, thus this Java file was ignored before. But, now that you converted this file, it forces you to also resolve/suppress any warnings that might have existed on this file. Otherwise, you won't be able to compile the project. This is done as such by design.

So, in order for you to overcome this, you would need to invest some additional time in resolving/suppressing all the warnings that exist on this newly converted Kotlin file. In this case, you might want to:

Resolve the getSerializableExtra and getSerializable warnings on `onCreate(...).
Suppress the unused warning on onCommentChanged(...).
Suppress the DEPRECATION warnings everywhere else.

I created a patch for your convenience (see below), just to give you a quick idea on how to deal with the allWarningsAsErrors configuration.

WordPress-Android-18432_issue-17260__Changing_CommentsDetailsActivity_to_Kotlin.patch

As such, switching the allWarningsAsErrors flag to false isn't allowed and wouldn't pass code review. If it helps, you can temporarily set this flag to false, just to get the project to temporarily compile, but then it needs to go back to true, when all the warnings are resolved/suppressed.

Is there any other way to fix this ?

I hope the above gave you a better idea on how to properly convert a Java class to Kotlin and what would that take. Let me know if you have any more questions, and again, thank you so much for helping modernizing our codebase, your work is very welcome. 🙇

PS: I recommend you redoing this PR of your, with a proper Java to Kotlin conversion this time (as explained above) and then resolving/suppressing all warnings, that is, before processing to you manual testing this screen for correctness purposes and open the PR.

shifli7 commented 1 year ago

Hi @ParaskP7,

PR - CommentsDetailsActivity Code Conversion to Kotlin

I Re-worked on above PR, I have followed as per your steps for converting the CommentsDetailsActivity.java in Java-to-Kotlin and also I checked this file is working fine.

Please check above mentioned PR. Did i want to change anything ?

ParaskP7 commented 1 year ago

👋 @shifli7 and thanks on re-working the above PR into opening a new PR, this is great!

I took another at your changes and I have the following for you:

Praise (❤️): Thanks for using the Extra commit for .java > .kt renames option, it makes reviewing the first Kotlinization commit very easy.
Warning (⚠️): You left a lot of !! everywhere within this CommentsDetailActivity class.
- One-by-one, per field or parameter, those need to be resolved into an equivalent solution that doesn't utilize !!.
- In the rare case that you would need to keep !!, then let that be, but do note the why somewhere, a code comment, commit or PR description.
Warning (⚠️): This mCommentsStoreAdapter field is Dagger related field (see @Inject). As such, it can and needs to be re-written as: @Inject lateinit var mCommentsStoreAdapter: CommentsStoreAdapter
Warning (⚠️): The getSerializable(...) and getSerializableExtra(...) related deprecations (x4) need to be resolved, not suppressed, just like I showcased within the patch I shared with you in my previous comment.
Warning (⚠️): Running Detekt (./gradlew detekt) fails the build on a couple of warnings that need to be resolved (like ReturnCount and MaxLineLength). Make sure that you can run both, Lint (./gradlew assembleJetpackWasabiDebug) and Detekt, and that they succeed, that is, before pushing your changes.
Minor (🔍): Consider putting the companion object block back at the bottom. It is recommended for such blocks to be at the bottom of a class, instead of them being at the top.

Let me know when you are done with the above so I can take another look. 🙏

Also, I noticed that the PR description is empty (⚠️), especially the To test section, which is the most important bit of information here. Note that without that, a reviewer cannot be confident that you as the author have manually tested this change via launching the app and navigating to the relevant screen, then playing a bit with it, just to make sure it works as expected. Also, without that piece of information a reviewer cannot pick-it-up and follow the To test instructions in order to proceed with some more testing on their side, that is, before approving the solution.

Does that make sense? 😊

FYI: I know that you did not expect the block of comments above, it might even sound a bit too much, but unfortunately we need to be very thorough with any change that enters our codebase, just to avoid any potential regression and additional tech debt work later on. As such, I hope the above advise will prove useful to you, not only when working with our codebase, but any such codebase for that matter. 🫂

PS: I also know that we will end-up becoming aligned very soon. When that happens, any future such change will become much easier for both, you as the author and anyone else reviewing and testing your change. 💯

shifli7 commented 1 year ago

Hi @ParaskP7,

PR - resolving above mentioned warnings

I have completed above mentioned point this PR and also updated the description for PR.

Warning (warning): You left a lot of !! everywhere within this CommentsDetailActivity class. One-by-one, per field or parameter, those need to be resolved into an equivalent solution that doesn't utilize !!. In the rare case that you would need to keep !!, then let that be, but do note the why somewhere, a code comment, commit or PR description.

I have removed some of the force unwrap in CommentsDetailsActivity, but some place it need to add !! this. so please check above mentioned PR and let me know can i use lateinit or if there this any other workaround or what i want to add as note.

ParaskP7 commented 1 year ago

👋 @shifli7 and thanks for updating #18480, good work!

I have removed some of the force unwrap in CommentsDetailsActivity, but some place it need to add !! this. so please check above mentioned PR and let me know can i use lateinit or if there this any other workaround or what i want to add as note.

I took another at your changes and I have the following for you:

Suggestion (💡): You have suppressed ReturnCount but consider resolving that instead. Also, there are lots !! references that could be resolved instead. FYI: Below, I am providing you with a code snippet for you to take a look and apply accordingly (see below). PS: Please take a look to understand it first, that is, before applying it. This way you'll get some worth out of it for future reference on resolving similar problems.

Code Snippet

```Kotlin private fun updateComments() { val site = mSite val statusFilter = mStatusFilter val offset = mAdapter?.count when { mIsUpdatingComments -> AppLog.w(AppLog.T.COMMENTS, "update comments task already running") !NetworkUtils.isNetworkAvailable(this) -> ToastUtils. showToast(this, getString(R.string.error_refresh_comments_showing_older)) !mCanLoadMoreComments -> AppLog.w(AppLog.T.COMMENTS, "no more comments to be loaded") site == null || statusFilter == null || offset == null -> AppLog.w(AppLog.T.COMMENTS, "site, statusFilter or offset is null") else -> { mCommentsStoreAdapter.dispatch( CommentActionBuilder.newFetchCommentsAction( FetchCommentsPayload(site, statusFilter, COMMENTS_PER_PAGE, offset) ) ) mIsUpdatingComments = true setLoadingState(true) } } } ```

Suggestion (💡): Consider replacing all mAdapter!! references with mAdapter? and then checking for equality (isAddingNewComments(commentList) == true) or nullability (commentIndex != null) when needed.
Suggestion (💡): Consider replacing all mOnPageChangeListener!! references with an equivalent mOnPageChangeListener?.let { ... } solution to resolve those nullability issues.

Let me know if the above make sense and then I can take another look at the final outcome, plus test it before merging. 🚀 🤞 🍀

FYI: Before or after pushing your changes don't forget to update you branch with the latest changes from trunk.

Stanzin7 commented 1 year ago

is this issue resolved, I can give it a try. Thanks

ParaskP7 commented 1 year ago

👋 @Stanzin7 and thanks for taking an interest at helping us here! 🤩

If @shifli7 is okay with that too or won't reply within the next few days, I'll go ahead and assign this to you. How does this sound to you? 😊

ParaskP7 commented 1 year ago

👋 @Stanzin7 !

Having waiting few days already, feel free to give this a try, good luck and again, thanks for your interest at helping us here! 🍀

PS: I'll now go ahead and assign you to this issue.

Stanzin7 commented 1 year ago

@ParaskP7 Thanks so much. I will give it a try )

wordpress-mobile / WordPress-Android

Compiler Warnings as Errors - WordPress Module - AsyncTask #17260