Closed BenHenning closed 4 weeks ago
PTAL @adhiamboperes.
Note that this PR is pretty straightforward to review, but it's very large and caused my computer to have some issues when trying to view the actual changes. It probably is worth breaking this into chunks when reviewing it.
I think it'd also be completely fine to delegate review of most or all of it to other people. I've self-reviewed the changes and find them non-concerning, but I defer to whatever you want to do as the primary reviewer & approver of the PR.
Unassigning @adhiamboperes since they have already approved the PR.
Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!
@seanlip PTAL for owners-specific changes.
@BenHenning Owners approval does not seem to be required for this PR, unless I'm missing something?
Hmm not sure why I saw that originally @seanlip. Thanks for double checking!
Thanks @adhiamboperes for the review!
Enabling auto-merge now that this is up-to-date with the latest develop changes.
Explanation
Fixes part of #4120 Fixes part of #1051
Similar to #5400, this brings forward changes that would otherwise go in #4937 to simplify the transition to Kotlin 1.6.
Part of #4937 is introducing warnings-as-errors for both Kotlin and Java in order to reduce developer error, simplify the codebase, and minimize warnings when building (which can result in developer habits of ignoring warnings that might have real consequences to end users of the app). In order to keep the main migration PR smaller, this PR fixes all existing warnings and any new ones detected with the Kotlin 1.6 compiler that are not tied to Kotlin 1.5/1.6 API changes (those are part of #4937, instead). Fortunately, most of the changes could be brought forward into this PR.
Specific things to note:
ObservableViewModel
was subsequently updated, and may be something we could remove in the future now that it's no longer a Jetpack view model.Observer
callbacks in UI code).DrawerLayout.setDrawerListener
was replaced with calls toDrawerLayout.addDrawerListener
since the former is deprecated. This isn't expected to have a functional difference.when
cases were updated to be comprehensive (as this is enforced starting in newer versions of Kotlin even for non-result basedwhen
statements)._
per Kotlin convention.ExitSurveyConfirmationDialogFragment
involved removing parsing a profile ID. Technically this is a semantic change since now a crash isn't going to happen if the profile ID is missing or incorrect, but that seems fine since the fragment doesn't even need a profile ID to be passed.Runnable
callback rather than binding to a method (just to avoid passing the unusedView
parameter and to keep things a bit simple binding-wise).getDrawable
calls were updated to pass in aTheme
since the non-theme version is deprecated.verifyZeroInteractions
has been deprecated in favor ofverifyNoMoreInteractions
, so updates were made in tests accordingly.ExperimentalCoroutinesApi
andInternalCoroutinesApi
have been deprecated in favor of a newerOptIn
method (which can actually be done via kotlinc arguments, but not in this PR). Thus, they've been outright removed in cases where not needed, and otherwise migrated to theOptIn
approach where they do need to be declared.toSet()
conversion for iterable operations when it's more performant, so some of those cases (where noticed) have been addressed.BundleExtensions
was updated to implement its own version of the type-basedgetSerializable
until such time asBundleCompat
can be used, instead (per #5405).NetworkLoggingInterceptorTest
was majorly reworked to ensure that the assertions would actually run (runBlockingTest
was being used which is deprecated, and something I try to avoid since it's very difficult to write tests that use it correctly). My investigations showed that the assertions weren't being called, so these tests would never fail. The new versions will always run the assertions or fail before reaching them, and fortunately the code under test passes the assertions correctly. Ditto forConsoleLoggerTest
.SurveyProgressController
were reworked to have better typing management and to reduce the need for nullability management.UrlImageParser
. See file comments & links in those comments for more context.BundleExtensionsTest
had to be changed sincegetSerializableExtra
is now deprecated. We also can't update the test to run SDK 33 since that requires upgrading Robolectric, and Robolectric can't be upgraded without upgrading other dependencies that eventually lead to needing to upgrade both Kotlin and Bazel (so it's a non-starter; this is a workaround until we can actually move to a newer version of Robolectric).ClickableAreasImage
.ConceptCardRetriever
(createWrittenTranslationFromJson
) and some other small cleanup/consolidation work happened in that class.TopicController
for JSON loading to better manage nullable situations, and to try and make the JSON loading code slightly more Kotlin idiomatic.Note that overall the PR has relied heavily on tooling to detect warnings to fix, and automated tests to verify that the changes have no side effects.
Note also that this PR does not actually enable warnings-as-errors; that will happen in a downstream PR.
Essential Checklist
For UI-specific PRs only
N/A -- While this changes UI code, it should change very few UI behaviors and only failure cases for those it does affect. It's largely infrastructural-only and falls mainly under refactoring/cleanup work.