Detekt - Resolve/Suppress All Baseline Warnings - Long methods warnings_ MediaPickerActivity - #17278
Conversation
ParaskP7
left a comment
ParaskP7
left a comment
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 #17283 that I just pushed on the main repo.
Below, I am going to leave you a few notes that I would love for you could take a look at, think about them and update the PR and code accordingly, please take this as an opportunity to learn so that we get better with every PR that will follow:
- PR Description Related:
- Suggestion (💡): You could be more specific about the commits you resolve, for example do this instead:
1x LongMethod (Resolve: e06b430 + 6086e9f) - Suggestion (💡): On your
To testsection (3rd bullet point) you specify that one would need to test every screen that related to these changes. Although this testing instruction is good, it is too vague. In this case, you could be more specific and give targeted instructions on how to test the screen associated with theMediaPickerActivityrelated screen. This way, you will give the reviewer an easy step-by-step guide on what to test and how. This will also increase the code reviewer's confidence that you took the time to do this testing as well. - Suggestion (💡): For the regression notes, on the potential unintended areas of impact section, try to think about them and if you can't think of any of don't believe there are any, at least leave a
N/A, just like you did for the automated tests section further below.
- Suggestion (💡): You could be more specific about the commits you resolve, for example do this instead:
- Commit Related:
- Suggestion (💡): Try being as descriptive with your commits as possible. For example, instead of
refactor for takeAPhotoyou could write something likeRefactor: Extract take a photo method for media picker activity(a max72chars per line). Note that I used theRefactoraction to indicate what this commit is about (other kinds of actions could beAnalysis,Build,Release,Feature, etc), but then, I was also specific about what kind of a refactor is that, in this case anextraction(other kinds of refactors could be amove,remoe,rename,optimize, etc).
- Suggestion (💡): Try being as descriptive with your commits as possible. For example, instead of
- Code Related:
- Warning (
⚠️ ): The main goal of this PR is to resolve this specific baseline warning and as such removing it from the baseline.xml Detekt related file. As such, although you did resolve the warning by extracting the code into newly created methods, you didn't follow-up with removing that specificLongMethod:MediaPickerActivityline from it. Also, note that with this refactor that you did you also resolved theNestedBlockDepth:MediaPickerActivity. As such, this line can be removed as well, with one change, you solved another problem too, kudos. - Suggestion (💡): Now that you extracted these code into newly created methods, you could also consider to further optimize this, by removing the extra braces from the
whenentries, thus leaving the code in an even better state than before. An extra commit like that will do:
- Warning (
TAKE_PHOTO -> takeAPhoto()
IMAGE_EDITOR_EDIT_IMAGE -> data?.let { editImageIntent(it) }
else -> data
I hope the above will help you instead of overwhelm you, as this is not my intention here. 🙏 No matter, I apologize in advance if the above overwhelms you anyway. I just hope you now understand why I am insisting in taking this slow, making small changes and progress accordingly, that is, after we discuss and improve on the overall process.
PS: Note that this MediaPickerActivity.onActivityResult(...) method got slightly updated already, on trunk, and via another refactor that happened few days ago (by me). This change added the @Suppress("DEPRECATION", "LongMethod", "NestedBlockDepth") and removed these violations from the baseline.xml that I specified in my above warning (trunk, you would need to update the suppress annotation to @Suppress("DEPRECATION") instead of removing the no longer existing (in current trunk) baseline lines. Does that help you? 😊
👋🏾 @ParaskP7 thank you once again for documenting your observation and suggestion. |
ParaskP7
left a comment
ParaskP7
left a comment
There was a problem hiding this comment.
👋 @Hoossayn !
👋🏾 @ParaskP7 thank you once again for documenting your observation and suggestion.
Again, thank YOU for helping us, I am merely here to support you on your journey and help you become productive with us. 🙇
I reviewed your update and everything LGTM, I just left a minor (🔍) for you to consider, but apart from that, which is optional anyway, everything looks okay and ready to be merged, I'll go ahead and approve this PR now. 🎉
Also you last point about making changes to MediaPickerActivity.onActivityResult(...) in the trunk. Did pulled the branch and there was no changes. or maybe perhaps i did't understand you properly
So, what I was trying to explain is the fact that the version of MediaPickerActivity.onActivityResult(...) in this PR is now different to the version currently on trunk, which has this set of extra suppressions by now:
@Suppress("DEPRECATION", "LongMethod", "NestedBlockDepth")
That, and the fact that this PR has now conflicts (see config/detekt/baseline.xml), means that it might be better for you to update this PR with the latest trunk and resolve any potential conflicts. Then, we will be able to re-trigger CI on it and eventually merge it.
Does that makes sense? 🙏
| TAKE_PHOTO -> takeAPhoto() | ||
| IMAGE_EDITOR_EDIT_IMAGE -> data?.let { editImageIntent(it) } | ||
| else -> data | ||
|
|
There was a problem hiding this comment.
Minor (🔍): Consider removing the extra empty line here.
There was a problem hiding this comment.
@ParaskP7 fix this lint error, also think i should mention that i am usually unable to see whatever you're trying to show me with the buildkite link 🫣, probably some set up i haven't done 👀
There was a problem hiding this comment.
👋 @Hoossayn and thank you for this fix! 👍
...think i should mention that i am usually unable to see whatever you're trying to show me with the buildkite link 🫣, probably some set up i haven't done 👀
Ah, probably you don't (and maybe can't) access Buildkite on your side, I'll confirm that and get back to you.
As such, and until then, I suggest you do the following, observe the CI checks and see what is failing, then try to run in on your side to observe the failure locally as well. For example, when you see CheckStyle failing you could run ./gradlew checkstyle locally and see it's output. You can also check the .buildkit/pipeline.yml file for all the other checks and how you could trigger them locally.
Another tip I can recommend for you to be doing before pushing any code, is to at least run these check locally first:
- CheckStyle:
./gradlew checkstyle - Detekt:
./gradlew detekt - Lint:
./gradlew lintWordPressWasabiDebug - Test:
./gradlew testWordPressWasabiDebugUnitTest
The above local checks will get you to almost 99% confidence, that is, in terms of the fact that our CI checks will also succeed afterwards and thus help you avoid this ping-pong that is happening atm.
Let me know if the above is helpful to you. 🙏
There was a problem hiding this comment.
👋🏾 @ParaskP7 Above check builds successfully except the last one, which i checked against trunk and was falling too
|
|
👋 @Hoossayn !
Ok, I got the issue here. You need to update your forked repo Let me know if the above helps you. 🙏 |
…Warnings_long_method_media_picker # Conflicts: # config/detekt/baseline.xml
👋🏾 @ParaskP7 |
…Warnings_long_method_media_picker # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/mediapicker/MediaPickerActivity.kt
|
👋 @Hoossayn !
Yea, using the terminal can be doubting. However, FYI, if you are not comfortable, you can also run these checks from your IDE as well, just check the
Good to know. 👍
I did take another look and I am afraid, as you will see from your changes, that the merging with the I also saw that you probably resolved the Just like my suggest with our previous such PR, I recommend we start again with a fresh PR, close this one, that is, after you update you fork version of |
👋🏾 @ParaskP7
|
|
👋 @Hoossayn !
Yes, you are right, it wasn't there before, but the codebase is alive and keeps being updated/improving with every day passing by, that's the beauty of it! 😃 You can always try to figure out why a specific changed happened using Now, if you are wonder why I moved the I hope the above clarifies things for you, let me know if you need additional clarification. 🙏
Yes, this is correct, as per my above answer, that PR and commit was responsible for that change. So, now, when you create a new PR that will again resolves these Does this help? 🙏 |
|
New Pull Request here -> #17355 |
2 participants
Parent: #17010
This PR resolved/suppress all complexity related LongMethod warnings for the WordPress module (see docs here):
1x LongMethod (Resolve: e06b430 + 6086e9f)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.
STEPS
Go to the menu tab
click on media option
click on the upload media button or the (+) button at the top right corner
select choose from device
select an image
Regression Notes
Potential unintended areas of impact
can't think of any
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.