Fixing Gradle 5.4.1 lint errors

[oguzkocer]

This is a draft PR which attempts to fix the lint errors caused by the Gradle 5.4.1 upgrade in #10583. I've decided to separate this into its own PR so it's easier to discuss the changes and work together on them. It also makes it clearer what's holding up the Gradle upgrade.

To test:
We'll probably need to briefly go through changed screens, it's hard to say before the PR is complete especially since it's related to lint issues.

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[oguzkocer]

@malinajirka Replying this comment here:

UseSparseArrays - I'd consider suppressing this warning at a project level or decreasing its severity to info. Sparse arrays can cause weird behavior in unit tests and imho the performance benefits aren't significant anyway. Wdyt?

It might be better to try to use the SparseArray first to see if we come across any issues and if not, use that just so we don't have one more thing we are suppressing. Especially with this, I am worried that we'll forget why we even have it in the first place. However, if using it creates any issues, we can go ahead with the suppression. Would that work for you?

StaticFieldLeak - This set of errors is pretty weird as they should be suppressed through the lint-baseline.xml. Not sure what's going on there 🤷‍♂

Yeah, it looks like we have it suppressed there 🤔 Maybe we need to change the baseline? Or we could individually suppress them and open an issue to fix them?

DefaultLocale, DiffUtilEquals - These seem like issues we should probably fix 🤔

I've attempted to fix DefaultLocale in 41593d6, however capitalize is still giving me lint errors as if I am not passing the locale to it. I checked the Kotlin function and it feels like it should work. I've also had to add ExperimentalStdlibApi because of capitalize. Not really sure what to do here, do you think you can also take a look at this one? Please don't spend too much time on it though, I already wasted so much already :(

I've created a PR in FluxC to fix the DiffUtilEquals issue. wordpress-mobile/WordPress-FluxC-Android#1398

I am going to take a break from this PR for today and work on something more useful 😩

[malinajirka]

It might be better to try to use the SparseArray first to see if we come across any issues and if not, use that just so we don't have one more thing we are suppressing. Especially with this, I am worried that we'll forget why we even have it in the first place. However, if using it creates any issues, we can go ahead with the suppression. Would that work for you?

Sure, we can give it a try - I'd just suggest using SparseArrayCompat which will behave the same on all version of Android.

Yeah, it looks like we have it suppressed there 🤔 Maybe we need to change the baseline? Or we could individually suppress them and open an issue to fix them?

I wouldn't create issues for them as imho they are not worth fixing - they are all over the place. I added them to the baseline and increased the severity to error so no-one ever introduces this issue again, but I wasn't planning on fixing them.

Maybe we need to change the baseline?

Yep, I'd try to add them to the baseline again - perhaps the syntax has changed a bit and that's why the suppression doesn't work.

I've attempted to fix DefaultLocale in 41593d6, however capitalize is still giving me lint errors as if I am not passing the locale to it. I checked the Kotlin function and it feels like it should work. I've also had to add ExperimentalStdlibApi because of capitalize. Not really sure what to do here, do you think you can also take a look at this one? Please don't spend too much time on it though, I already wasted so much already :(

I'll look into it.

[oguzkocer]

Sure, we can give it a try - I'd just suggest using SparseArrayCompat which will behave the same on all version of Android.

I've added SparseArrayCompat and androidx version of LongSparseArray in 1febc41.

I wouldn't create issues for them as imho they are not worth fixing - they are all over the place. I added them to the baseline and increased the severity to error so no-one ever introduces this issue again, but I wasn't planning on fixing them.

I was actually thinking a single issue with maybe a checklist or something. I am not sure if I agree with they are not worth fixing, but I can see us not being able to work on them unless someone picks it up during a hack week.

Yep, I'd try to add them to the baseline again - perhaps the syntax has changed a bit and that's why the suppression doesn't work.
I'll look into it.

Thank you! I think these 2 are the only remaining bits except for the equals discussion that's going on in FluxC. Let me know what, if anything, you find out!

[malinajirka]

I've added SparseArrayCompat and androidx version of LongSparseArray in 1febc41.

👍

I was actually thinking a single issue with maybe a checklist or something. I am not sure if I agree with they are not worth fixing, but I can see us not being able to work on them unless someone picks it up during a hack week.

What I meant by "they are not worth fixing" was that they are not straight-forward fixes and the cost of fixing those issues might out-weight the benefits of fixing them. However, I like having a single issue with a list which we can fix during a hack week or something 👍.

I looked into the issue with lint-baseline.xml and fixed it in issue/gradle-5-lint-errors-static-field-leak (I didn't want to commit changes into your branch, but feel to merge them) -> the only issues remaining in that branch are "equals" and "capitalize".

---

I've quickly tested SiteCreation, PostList + Upload, PagesList and Stats => Everything seems to be working as expected. Feel free to merge this PR when you fix/suppress the remaining issues.

Regarding the #10583 and wordpress-mobile/WordPress-FluxC-Android#1397 PRs -> since I tested this PR I don't think it makes sense to test it again after the merge. However, I'd feel safer if someone from the Gutenberg team verified that the changes don't break their flow. Other than that I think they are ready to be merged (btw I pinged Gutenberg mobile team).

P.S. Good job fixing all the issues and upgrading the gradle version ;)!!!

[oguzkocer]

I looked into the issue with lint-baseline.xml and fixed it in issue/gradle-5-lint-errors-static-field-leak

@malinajirka Thank you for taking care of this! I cherry-picked your commit into this branch and deleted the remote branch.

I've fixed the equals issue by manually implementing an equals. I thought a lot about just suppressing the issue, however in some ways the lint is right. If we suppress the error, it'll also suppress if there is a legitimate reason for it. Consider the case where we add one more subclass to that sealed class and we don't implement equals for it. The == comparison will end up doing an identity check which is not what we want. Even though the lint error is wrong in this instance, because we are using data classes and equals is implemented for them, it could be right in the future and we'd remove that fail-safe by suppressing it. That's why in my original PR I mentioned that it should ideally understand that all subclasses are data classes and this error is not valid. Anyway, I think it's a very small trade-off doing a manual comparison especially if it's written as in 50c0f60 because it ensures a compiler error if we add another subclass to it. Let me know your thoughts on this.

I've suppressed the default locale error for the String.capitalize(Locale) method by adding a wrapper and documentation to it. Please check out the documentation and the reasoning for why I opted for this wrapper extension. 6b571a9

I'd feel better if you review these 2 commits before we merge it. I'd also like to wait until we figure out the Gutenberg issue in the main branch so we can merge all of them at once. If there are any more lint issues, it'd be better to fix them here and if there are other issues it'd be better to fix them in the main branch so it's easier to review/test changes.

@malinajirka Thank you for all your help and the reviews! 🙇

[malinajirka]

I've fixed the equals issue by manually implementing an equals.

👍 Your explanation makes a lot of sense and this is a great solution! Thank you

I've suppressed the default locale error for the String.capitalize(Locale) method by adding a wrapper and documentation to it.

👍 perfect!

I'd feel better if you review these 2 commits before we merge it.

The new changes look great;). As you mentioned lets wait until we figure out the Gutenberg issue.

[oguzkocer]

Merging this approved PR to get #10583 ready since we addressed all the remaining issues for the Gradle upgrade.