Detekt - Resolve/Suppress All Baseline Warnings - Long methods warnings #17198
Conversation
|
👋 @Hoossayn ! Thank you so much for contributing yet another PR to this repo, this is awesome! 🙇 In addition to the code changes, how would you like us to start working on improving on the PR description itself? This is very important to us as it helps with the organization aspects of working on an issue. Also, doing so is very helpful to reviewers as it informs them on every aspect of the PR, without having them be thinking about any missing aspect themselves, thus increasing their level on confidence on your code changes and the testing to verify that everything is still working as expected. In addition to the above, you could also start creating new issues, based on the parent issue and start informing us on the tasks you are going to be working next. This way we will be aware what to expect, possibly even help and guide you, even before a future PR is created. Wdyt? PS: For inspiration, you could also take a look at this FluxC relevant parent issue, its sub-issues (for example this), the corresponding PR based on these sub-issues (for example this), and based on the later, you could try and follow a similar PR description approach. FYI: To help you out a bit, below is how I would described this PR: Parent: #17010 This PR resolved/suppress all
To test:
Regression Notes
<SOME 123> functionality is not workings as expected.
See
N/A PR submission checklist:
|
There was a problem hiding this comment.
👋 @Hoossayn !
Thank you so much for this PR. Your contributions are always welcome! 🎉
I did an initial quick review of this PR, along with triggering CI on it via a draft PR that I just pushed on the main repo.
I'll do a more thorough review on the code changes after you follow-up on the bellow initial comments from my side:
- Blocker (🚫): Please take a look as the CI checks that are failing on a couple of occasions and try to fix those issues:
- The Dependency Tree Diff check is failing due the conflicts that exist in this PR. Please take a look at that and resolve the conflicts first.
- The detekt check is failing due to the 6 new
LongParameterListissues that start appearing as part of your refactoring attempt to solve theLongMethodissues. Please take a look at that and resolve these newly created Detekt issues. You can run./gradlew detektlocally on your side before pushing your changes to verify that Detekt runs successfully and there is no build failures associated with this task.
- Suggestion (💡): For all these 15 files you touched, you did that on a single commit basis. I could recommend to you, for this (or next time), to spit such changes into at least 15 commits, each commit to deal with a specific refactor. This way it would be easier for the reviewer to review each change in isolation. On some occasions, I would personally go as far as splitting on of these potential 15 separate commits even further, into more commits. For example, for the
MediaPickerActivity.ktfile change, I would have first extracted thetakeAPhoto()function, then committed this change, then, I would have extracted theeditImageIntent(...)function, then committed another change. Thus for that specific file, I would have ended up with 2 commits instead of 1 (out of the 15). One would do this in order to make it easier for the reviewer to do the code review diffing afterwards, in order for them to verify that everything was done appropriately, most probably, in an automated way, so that to avoid any hidden surprises. Wdyt? 🤔
config/detekt/baseline.xml
Outdated
| <ID>LongMethod:StatsWidgetConfigureFragment.kt$StatsWidgetConfigureFragment$override fun onViewCreated(view: View, savedInstanceState: Bundle?)</ID> | ||
| <ID>LongMethod:SuggestionActivity.kt$SuggestionActivity$private fun initializeActivity(siteModel: SiteModel, suggestionType: SuggestionType)</ID> | ||
| <ID>LoopWithTooManyJumpStatements:RemoveMediaUseCase.kt$RemoveMediaUseCase$for (mediaId in mediaIds) { if (!TextUtils.isEmpty(mediaId)) { // make sure the MediaModel exists val mediaModel = try { mediaStore.getMediaWithLocalId(Integer.valueOf(mediaId)) ?: continue } catch (e: NumberFormatException) { AppLog.e(AppLog.T.MEDIA, "Invalid media id: $mediaId") continue } // also make sure it's not being uploaded anywhere else (maybe on some other Post, // simultaneously) if (mediaModel.uploadState != null && mediaUtils.isLocalFile(mediaModel.uploadState.toLowerCase(Locale.ROOT)) && !uploadService.isPendingOrInProgressMediaUpload(mediaModel)) { dispatcher.dispatch(MediaActionBuilder.newRemoveMediaAction(mediaModel)) } } }</ID> | ||
| <ID>LoopWithTooManyJumpStatements:RemoveMediaUseCase.kt$RemoveMediaUseCase$for (mediaId in mediaIds) { if (!TextUtils.isEmpty(mediaId)) { // make sure the MediaModel exists val mediaModel = try { mediaStore.getMediaWithLocalId(Integer.valueOf(mediaId)) ?: continue } catch (e: NumberFormatException) { AppLog.e(AppLog.T.MEDIA, "Invalid media id: $mediaId") continue } // also make sure it's not being uploaded anywhere else (maybe on some other Post, // simultaneously) if (mediaModel.uploadState != null && mediaUtils.isLocalFile(mediaModel.uploadState.toLowerCase(Locale.ROOT)) && !uploadService.isPendingOrInProgressMediaUpload(mediaModel)) { dispatcher.dispatch(MediaActionBuilder.newRemoveMediaAction(mediaModel)) } } }</ID> |
There was a problem hiding this comment.
Minor (🔍): Consider adding an extra space to this LoopWithTooManyJumpStatements:RemoveMediaUseCase related line as this file is not incorrectly formatted due to this change.
👋🏾 @ParaskP7 thank you for your input, appreciate it greatly, i have done some changes to follow suite to your suggestion, kindly let me know if there's any further changes that's required |
|
👋 @Hoossayn ! Thank you for the changes and for following the suggestions, much appreciated. 🙇 PS: I am a bit busy amt, but I'll be sure to get to review this changes first thing next week. |
|
👋 @Hoossayn ! I have started reviewing your PR today. 🔍
On your PR description, you are point that this #15 PR, why is that? 🤔 FYI: Usually, a
As per this comment on mine, let's start creating issues that are going to then be then associated with the PRs you are going to be creating so that we can track the parent #17010 better and also make sure that the |
There was a problem hiding this comment.
👋 @Hoossayn !
I did a quick review of the new commits you pushed, once more, thank you for helping! 🥇
As I have lots of code review comments based on the code changes I am seeing so far, and because I would hate to overwhelm you with them, I would recommend for us to take a small step back, close this PR and do the following instead:
- Open another PR, which will only try to fix the
LongMethodwarnings for these 3org.wordpress.android.ui.mediapickerrelated classes:MediaPickerActivityMediaPickerFragmentMediaPickerViewModel
- While doing the above, try to commit your changes as soon as possible, at the basis, I would expect 3 commits, one per class above. Even further, I would like you to do the following, commit per refactor. For example, for the
MediaPickerFragmentclass I suggest you do the following refactors into separate commits:setUpRecyclerView(...)navigateEvent(...)editMedia(...)previewMedia(...)previewUrl(...)
- While working on each refactor commit above, please make sure you are not creating any new warnings, and if possible resolving all existing. For example, for the
MediaPickerFragment.navigateEvent(...)refactor, you could resolve the lambda argument leftover warnings, by moving the lambda argument out of the parentheses (AS could help you with that). And then, removingnavigationEventaltogether, replacing it withit, leaving this a one-liner, like this:viewModel.onNavigate.observeEvent(viewLifecycleOwner) { navigateEvent(it) }
With that, I think we will end-up moving faster, at least commit your changes to trunk quicker. Wdyt? 🤔
hey 👋🏾 @ParaskP7, Been sorry for just getting to this, been tied up with other things
|
|
👋 @Hoossayn !
Hey, please don't apologize, we are not in a rush here, not at all. At this point, it is better we do it right than hurry into it. So, pretty please feel free to complete this at your own pace and when you have some additional time on your side.
Happens to the best of us! 😅
The way I was thinking of you informing us on what you are going to be working next is via a detailed GitHub issue. You could use that to plan all this work. As an example, see here. You will notice that I created this issue first, noting all the work I should be doing next and then spitting this work into sections that I could pick up to work on. |
|
👋 @Hoossayn !
Thank you, for your time! 🥇
Great, I'll take a look at that today! 🚀 In the meanwhile, I will be closing this PR in favor of this #17278 and any subsequent PRs that you are going to be working next. I hope that's okay with you. |
Parent: #17010
This PR resolved/suppress all complexity related LongMethod warnings for the WordPress module (see docs here):
15x LongMethod (Resolve: https://github.com/wordpress-mobile/WordPress-Android/pull/17198/commits)To test:
There is nothing much to test here.
Verifying that all the CI checks are successful should be enough (especially the detekt check).
However, if you really want to be thorough, you could smoke test the WordPress and/or Jetpack apps to verify that everything works as expected on every screen that relates to these changes.
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
See
To testsection above.What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.