-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Kotlin - fix nullability #12047
Conversation
@david-allison All checks are failing. |
This is before converting 'Utils.java' **Collection:** * setUserFlag * genCards * cardCount * emptyCardReport * updateFieldCache **BaseSched:** * remFromDyn **Anki2Importer:** * _writeDstMedia
d15cfcd
to
0ac9bcd
Compare
import com.ichi2.utils.HashUtil | ||
import com.ichi2.utils.HtmlUtils | ||
import com.ichi2.utils.JSONObject | ||
import com.ichi2.utils.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this was done by Android Studio, do you want to revert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is apparently a really contentious area.
I read through a couple threads, and this one (with a focus around here pinterest/ktlint#48 (comment)) was informative for me.
It appears that star imports are actually a more Kotlin-y thing (IntelliJ / Android Studio add them by default, the language implementation uses them, and the package is the "unit" of import in Kotlin more than a single file like Java)
So I'm leaning towards not caring about star imports, and ktlint in it's default configuration also does not care, so I'll let this go.
Note that I'm injecting opinion here, it's a matter of taste. Feel free to disagree. If most people disagree, it's possible to configure ktlint to disallow star imports from what I understand, and that's the thing to do so these rules are enforced by tooling not convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - long comment on star imports which I'm willing to defer to now - it either gets put into our tooling or we allow them (which would be fine by me)
Passes local re-integration
This is useful before converting 'Utils.java'
Collection:
BaseSched:
Anki2Importer:
How Has This Been Tested?
Unit tested
Checklist
if
statements)