Complete Kotlin Migration 🚀 🚀 by david-allison · Pull Request #12312 · ankidroid/Anki-Android

david-allison · 2022-09-04T07:37:14Z

Pull Request template

Purpose / Description

I wanted to get this one in for discussion - still blocked on a 3 files, which cause a CI break:

Fixes

We don't have an issue for this

Approach

remove Java only lint rules
remove migration related tooling + documents
remove @JvmOverloads
remove @NonNull/@Nullable + lint rule
plan to remove @JvmStatic and @JvmField

How Has This Been Tested?

CI only

Learning (optional, can help others)

Started in Configure Kotlin in the project and migrate UiUtil to Kotlin #8968 (2021-05-28)
Finished (sans cleanup) over a year later.

Checklist

Please, go through these checks before submitting the PR.

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

BrayanDSO · 2022-09-04T17:10:24Z

Just remembered this: an issue to replace all usages of TextUtils.isEmpty with kotlin methods can be created now

And remember to remove the wiki page about kotlin migration and Java code style

BrayanDSO · 2022-09-04T17:13:12Z

As some JvmStatic and JvmName will have to stay, like on protected static methods, and someone will have to go through each case, it would be nice to add comments on the code explaining why each annotation was kept

No longer necessary. API has been converted to Kotlin

No longer necessary. Anki-Android has been converted to Kotlin

No more Java in project PreferIsEmptyOverSizeCheck is now an IDE Lint

reason: No more Java in project * (Java)NonPublicNonStaticJavaFieldDetector: - Lint: NonPublicNonStaticFieldName * ConstantJavaFieldDetector: - Lint: ConstantFieldName

* Remove `InconsistentAnnotationUsage` + Tests * Remove unnecessary `@NonNull` and `@Nullable` annotations No longer needed as everything is in Kotlin

No longer necessary: all files are in Kotlin

david-allison · 2022-09-04T17:29:17Z

TextUtils.isEmpty

Added this in the PR, it actually goes further. If we remove all Android references from a class (of which TextUtils was a common one), then we can remove AndroidJUnit which can be up to a 100x speed increase with tests.

This gets better, as translations are the major cause, and I plan for these to go from Android -> Rust at some point

mikehardy · 2022-09-04T18:52:35Z

david-allison · 2022-09-04T19:10:33Z

I'd prefer this one was rebased - some of the changes (JvmOverloads have introduced issues which I fixed in testing, and would want a targeted revert if more issues appear, rather than reverting it all)

Arthur-Milchior

Good to go as far as code change go. Maybe would love slightly more explanation, but not blocking

Arthur-Milchior · 2022-09-04T19:44:36Z

                JUnitNullAssertionDetector.ISSUE,
                KotlinMigrationBrokenEmails.ISSUE,
                KotlinMigrationFixLineBreaks.ISSUE,
-                PreferIsEmptyOverSizeCheck.ISSUE,


If it's in IDE, I guess it won't be detected by the linter we use in CI, or did I understand it wrongly?
Fine to remove it anyway, assuming that it'll still be seen by people when writing code

This is code that we wrote ourselves for Java only. This has nothing to do with the IDE lint which will still run

Oh, I missed that it was java only anyway. So clearly, no reason to keep it.
My remark about IDE was an answer to the commit message, if I recall correctly, mentionning taht IDE detects this error in Kotlin.

I wonder whether it's right that it's only detected by IDE and not by the CI, but at least that's not a new problem then

Arthur-Milchior · 2022-09-04T19:50:38Z

@@ -1,243 +0,0 @@
-#!/bin/bash


Oops, I forgot to give it a licence.

Too late.

It's strange to see it go. Probably the only part where I was proactive in this migration.
First time ever I create a tool whose usage was not either a single day or expected to be a long future, but with a clear end.

Arthur-Milchior · 2022-09-04T20:04:02Z

    AnnotationTarget.VALUE_PARAMETER, AnnotationTarget.EXPRESSION,
    AnnotationTarget.FIELD, AnnotationTarget.PROPERTY, AnnotationTarget.LOCAL_VARIABLE,
-    AnnotationTarget.CONSTRUCTOR
+    AnnotationTarget.CONSTRUCTOR, AnnotationTarget.FILE


If I get this properly, it means that KotlinCleanup can now apply at the file level.
How does it relate to the plan to remove @JvmStatic. I assume it's because you are adding the @file: above. But I don't understand what is the point to applying this KotlinCleanup to current file. If it's just a todo list for the whole project, I don't believe annotation adds anything compared to just having a standard text file/wiki with a todo list

It's a to-do that means we can't remove KotlinCleanup before we handle the issue.

We could then tie each instance of file:KotlinCleanup to an issue on GitHub, but it's probably best to do them in one PR per annotation rather than waste time on overheard.

JvmOverloads wasn't hard to remove

Fine by me.

BrayanDSO

I think things won't crash, ao LGTM

I'd prefer this one was rebased - some of the changes (JvmOverloads have introduced issues which I fixed in testing, and would want a targeted revert if more issues appear, rather than reverting it all)

Agreed. If I were to squash something, it would be the docs: plan X commits, implementer's choice.

My comments are just notes. Do resolve them at will

This is no longer necessary (besides the API and a few preferences, where a Java XML inflater is used to call the constructor) Fixed a couple of cleanup comments referring to the annotation

**TextUtils** Once TextUtils is removed from a class, we may have no references to Android A non-Android class does not need AndroidJUnit, speeding up tests **JvmField/JvmStatic** Now all files are Kotlin, we only need a few `JvmField/JvmStatic` annotations for APIs which use Java reflection, the rest can go.

We no longer have Java files in the project

david-allison · 2022-09-04T20:36:18Z

Agreed. If I were to squash something, it would be the docs: plan X commits, implementer's choice.

done, commit message for re-review

david-allison added the Blocked by dependency Currently blocked by some other dependent / related change label Sep 4, 2022

david-allison added 6 commits September 4, 2022 18:19

chore: remove kotlinMigration-api.gradle

ee1a473

No longer necessary. API has been converted to Kotlin

chore: remove kotlinMigration.gradle

9aad6f8

No longer necessary. Anki-Android has been converted to Kotlin

chore: remove PreferIsEmptyOverSizeCheck

34af646

No more Java in project PreferIsEmptyOverSizeCheck is now an IDE Lint

chore: remove JavaFieldNamingPatternDetector

8e6548a

reason: No more Java in project * (Java)NonPublicNonStaticJavaFieldDetector: - Lint: NonPublicNonStaticFieldName * ConstantJavaFieldDetector: - Lint: ConstantFieldName

chore: remove Java NonNull/Nullable references

a289225

* Remove `InconsistentAnnotationUsage` + Tests * Remove unnecessary `@NonNull` and `@Nullable` annotations No longer needed as everything is in Kotlin

chore: delete Kotlin Migration related tools/docs

f78fabf

No longer necessary: all files are in Kotlin

david-allison force-pushed the finish-kotlin-migration branch 2 times, most recently from 32faa88 to 5418774 Compare September 4, 2022 17:29

This comment was marked as resolved.

Sign in to view

david-allison force-pushed the finish-kotlin-migration branch 2 times, most recently from 3cce257 to 479783c Compare September 4, 2022 18:46

david-allison marked this pull request as ready for review September 4, 2022 18:46

This comment was marked as resolved.

Sign in to view

david-allison added Needs Review and removed Blocked by dependency Currently blocked by some other dependent / related change labels Sep 4, 2022

Arthur-Milchior approved these changes Sep 4, 2022

View reviewed changes

Arthur-Milchior added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Sep 4, 2022

BrayanDSO approved these changes Sep 4, 2022

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Sep 4, 2022

david-allison force-pushed the finish-kotlin-migration branch from 479783c to 9cee3c1 Compare September 4, 2022 20:31

david-allison added 3 commits September 4, 2022 21:32

chore: remove most @JvmOverloads

45fa2c2

This is no longer necessary (besides the API and a few preferences, where a Java XML inflater is used to call the constructor) Fixed a couple of cleanup comments referring to the annotation

chore: jacoco - remove Java coverage

bc55f58

We no longer have Java files in the project

david-allison force-pushed the finish-kotlin-migration branch from 9cee3c1 to bc55f58 Compare September 4, 2022 20:35

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 4, 2022

Arthur-Milchior merged commit 3c37900 into ankidroid:main Sep 4, 2022

github-actions Bot added this to the 2.16 release milestone Sep 4, 2022

github-actions Bot removed the Needs Second Approval Has one approval, one more approval to merge label Sep 4, 2022

david-allison deleted the finish-kotlin-migration branch September 4, 2022 22:45

david-allison mentioned this pull request Sep 18, 2022

Remove config for JavaCompile in gradle #12431

Merged

thedroiddiv mentioned this pull request Nov 16, 2022

Creating an standard for steps and procedure to migrate from Java to Kotlin robolectric/robolectric#7817

Open

Uh oh!

Uh oh!

Conversation

david-allison commented Sep 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request template

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Learning (optional, can help others)

Checklist

Uh oh!

BrayanDSO commented Sep 4, 2022

Uh oh!

BrayanDSO commented Sep 4, 2022

Uh oh!

david-allison commented Sep 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

mikehardy commented Sep 4, 2022

Uh oh!

david-allison commented Sep 4, 2022

Uh oh!

Arthur-Milchior left a comment

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

david-allison Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Arthur-Milchior Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

david-allison Sep 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

BrayanDSO left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-allison commented Sep 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

david-allison commented Sep 4, 2022 •

edited

Loading

david-allison commented Sep 4, 2022 •

edited

Loading

david-allison Sep 4, 2022 •

edited

Loading

BrayanDSO left a comment •

edited

Loading