mozilla-lockwise / lockwise-android

Firefox's Lockwise app for Android
https://mozilla-lockwise.github.io/lockwise-android/
Mozilla Public License 2.0
623 stars 104 forks source link

Upgrade compile and target SDK to 29 #1243

Closed jonalmeida closed 3 years ago

jonalmeida commented 3 years ago

Fixes #1241

Testing and Review Notes

~I wasn't able to test these changes but I wrote some tests or fixed the ones that mattered.~ I did some testing but a round of QA for this with varying network connectivity would be great.

To Do

jonalmeida commented 3 years ago

I'm confused why this replaced #1242. The bulk of my review is there.

I believe there was some CI issues with a PR not running all the things unless it was from a branch part of the main project, so Stefan recommended a new PR that wasn't based on my fork.

There are 15 files out of 37 where you're changing to use requireView() and requireContext() and at least one file fixing semantically valid code of dubious quality. At this stage of the release cycle, I'd encourage those changes— mechanical as they are— to be done in a separate PR.

I separated the mechanical parts in the first commit and the logic part in the second (connectivity API changes).

fixing semantically valid code of dubious quality

It was typing fatigue really, but does not change the behaviour.


Merging the review comments from the old PR here:

Should this be at 28? or at 24? or 29? Either a comment in the code or here.

This would usually match the target version implicitly, but Robolectric does not currently target 29 - we had to do this in AC as well.

I'm not sure what to do about other comments, they seem more informational to me.