Non-null assertion cleanup PART 1 - #706
growabeard wants to merge 8 commits into
Conversation
andrewtavis
added
the
no-changelog
|
Thanks for the PR, @growabeard! Would you be able to fix the Ktlint issues? |
|
Egg on my face. I ran lintKotlin but ignored the output.. 👻 |
|
No stress, @growabeard! @prince-0408 and/or @Roniscend: Would either of you be able to do an initial review? How many parts do you think you'll do for the PRs, @growabeard? Just wondering if you have an impression :) |
|
I think just one more actually. I should have the second one ready this week. |
|
Sounds great, @growabeard! |
|
@prince-0408 can you recheck now? |
63bc9cb to
1ba26c4
Compare
| if (emojiEditorList != null) { | ||
| emojiEditorList!!.add(emoji) | ||
| if (emojiEditorList.isNotEmpty()) { | ||
| emojiEditorList.add(emoji) |
There was a problem hiding this comment.
Since emojiEditorList is now a non-null MutableList<String>, both branches perform the exact same operation (appending emoji). You can simplify this to:
| emojiEditorList.add(emoji) | |
| emojiEditorList.add(emoji) |
| } | ||
| } | ||
| emojiEditorList = null | ||
| emojiEditorList = emptyList<String>().toMutableList() |
There was a problem hiding this comment.
emojiEditorList.clear() (or emojiEditorList = mutableListOf()) is more idiomatic than emptyList<String>().toMutableList() and avoids allocating intermediate collections.
Also, inside commitEmojiEditorList(), the emojiEditorList.let { ... } block is redundant now that emojiEditorList is non-null and can be simplified.
|
|
||
| private const val PREFS_NAME = "recent_emojis" | ||
| private const val KEY_RECENT = "recent_emoji_list" | ||
| const val PREFS_NAME = "recent_emojis" |
There was a problem hiding this comment.
Consider making PREFS_NAME and KEY_RECENT internal const val instead of public const val so they don't leak into the public API outside the keyboard module, while remaining accessible to RecentEmojiHelperTest.
|
|
||
| recordRecentEmoji(context, "emoji1") | ||
|
|
||
| verify(exactly = 1) { mockEditor.putString(any(), any()) } |
There was a problem hiding this comment.
We can verify the exact key and emoji value stored instead of using any() to ensure the emoji is properly persisted when preferences are initially null:
| verify(exactly = 1) { mockEditor.putString(any(), any()) } | |
| verify { mockEditor.putString(KEY_RECENT, "emoji1") } |
| import org.junit.jupiter.api.Test | ||
|
|
||
| class AutocompletionHandlerTest { | ||
| private lateinit var looper: Looper |
There was a problem hiding this comment.
looper is declared and mocked in setUp(), but never used anywhere in this test class. It can be safely removed.
|
|
||
| val result = AutocompletionHandler.buildCompletions(typedWord, completions) | ||
|
|
||
| assert(result.isEmpty()) |
There was a problem hiding this comment.
For consistency with the other test classes in this PR and to avoid depending on the -ea JVM runtime flag for Kotlin's assert(...), please use JUnit 5 assertions here (e.g. assertTrue(result.isEmpty()) or assertEquals(emptyList<String>(), result)).
| // Unit Testing | ||
| // ========================== | ||
| testImplementation("org.junit.jupiter:junit-jupiter-api:$junit5Version") | ||
| testImplementation("org.junit.jupiter:junit-jupiter-params:${junit5Version}") |
There was a problem hiding this comment.
Nit: Can omit the curly braces ($junit5Version) to match the surrounding dependency lines.
3 participants
Contributor checklist
./gradlew lintKotlin detekt testcommand as directed in the testing section of the contributing guideDescription
This PR is fixing non-null assertions in a few classes, with tests accompanying the changes.
This is only the first PR in a series of work.
Related issue