KT-10974 Add Code Style: Import Layout Configuration Table #3336

gcx11 · 2020-04-25T18:33:03Z

https://youtrack.jetbrains.com/issue/KT-10974

fedochet

Thanks for your contribution @gcx11 ! Please, take a look at my commentary

Feel free to ask questions if something is not clear

fedochet · 2020-04-28T11:14:37Z

idea/formatter/src/org/jetbrains/kotlin/idea/core/formatter/KotlinPackageEntry.kt

+    }
+
+    fun matchesPackageName(otherPackageName: String): Boolean {
+        if (this == ALL_OTHER_IMPORTS_ENTRY || this == ALL_OTHER_ALIAS_IMPORTS_ENTRY) return true


isSpecial can be used here, I guess

fedochet · 2020-04-28T11:15:00Z

idea/formatter/src/org/jetbrains/kotlin/idea/core/formatter/KotlinPackageEntry.kt

+    }
+
+    val isSpecial: Boolean
+        get() {


You can rewrite it as an expression, without return

fedochet · 2020-04-28T11:16:28Z

idea/formatter/src/org/jetbrains/kotlin/idea/core/formatter/KotlinPackageEntryTable.kt

+        if (other !is KotlinPackageEntryTable) return false
+        if (other.entries.size != entries.size) return false
+
+        return entries.zip(other.entries).any { (entry, otherEntry) -> entry != otherEntry }


This looks like a regular == for lists

Also, it seems strange that when any package is not equals, equals returns true

fedochet · 2020-04-28T11:28:59Z

idea/formatter/src/org/jetbrains/kotlin/idea/core/formatter/KotlinPackageEntryTable.kt

+        for (entry in entries) {
+            val element = Element("package")
+            parentNode.addContent(element)
+            val name = if (entry == KotlinPackageEntry.ALL_OTHER_IMPORTS_ENTRY || entry == KotlinPackageEntry.ALL_OTHER_ALIAS_IMPORTS_ENTRY) "" else entry.packageName


isSpecial should work here too

fedochet · 2020-04-28T12:46:17Z

idea/formatter/src/org/jetbrains/kotlin/idea/core/formatter/KotlinPackageEntryTable.kt

+
+    operator fun contains(packageName: String): Boolean {
+        for (entry in entries) {
+            if (packageName.startsWith(entry.packageName)) {


This code is actually repeated in the matchesPackageName

fedochet · 2020-04-29T14:24:17Z

idea/formatter/src/org/jetbrains/kotlin/idea/core/formatter/KotlinPackageEntry.kt

+        return false
+    }
+
+    fun isBetterMatchForPackageThan(entry: KotlinPackageEntry?, path: ImportPath): Boolean {


I don't think that this KotlinPackageEntry? is necessary here, you can just check for null before passing it

@fedochet I don't think that would work

I would have to extract call to matchesPackageName as well otherwise we would end up selecting incorrect package in method bestMatchIndex

fedochet · 2020-04-29T15:59:24Z

idea/src/org/jetbrains/kotlin/idea/formatter/BaseKotlinImportLayoutPanel.kt

+
+                when (entry) {
+                    KotlinPackageEntry.ALL_OTHER_IMPORTS_ENTRY -> append(
+                        "all other imports",


Please, use ApplicationBundle.message here

@fedochet I couldn't find property with such text, does that mean I should create new one?

@gcx11 I am sorry, but it seems like I've misguided you a little bit about what messages should be externalized

It seems like the only message that should be externalized is the "Import aliases separately" message

You can add this message in this file and then use KotlinBundle.message to obtain it: https://github.com/JetBrains/kotlin/blob/f494b4ce11b3f950199242d035ee803ac7c59a57/idea/resources/messages/KotlinBundle.properties

The messages that you've fixed actually do not require fixing, because

setButtonComparator expects buttons names, which seems to be hardcoded, and replacing them with even slightly different messages will lead to broken (or just default) sorting

In the case of import all other imports or import aliases it would be better to have them in English, as it is more a code example then a UI message

I am sorry that you have to revert some of your changes back, I should have paid more attention to the code

If it is troublesome for you to revert those changes, please let me know here and I will just amend them into your commits myself before the merge (your authorship of the code will remain intact, but I will merge the PR by hand, not on the Github)

Of course, you are welcome to fix the problem yourself if you like to

@fedochet I have sent new commit that should address your last comment, would you mind to look at the PR again?

@gcx11 sure! I will take a look tomorrow

fedochet · 2020-04-29T15:59:40Z

idea/src/org/jetbrains/kotlin/idea/formatter/BaseKotlinImportLayoutPanel.kt

+            .setAddAction { addPackage() }
+            .setRemoveAction { removePackage() }
+            .setButtonComparator(
+                "Add",


Please, use ApplicationBundle.message here

fedochet · 2020-04-29T15:59:45Z

idea/src/org/jetbrains/kotlin/idea/formatter/BaseKotlinImportLayoutPanel.kt

+                entry != null && entry != KotlinPackageEntry.ALL_OTHER_IMPORTS_ENTRY && entry != KotlinPackageEntry.ALL_OTHER_ALIAS_IMPORTS_ENTRY
+            }.setButtonComparator(
+                ApplicationBundle.message("button.add.package"),
+                "Remove",


Please, use ApplicationBundle.message here

I am sure that there are some preexisting messages for this

fedochet · 2020-04-29T16:34:00Z

idea/src/org/jetbrains/kotlin/idea/util/ImportPathComparator.kt

+class ImportPathComparator(
+    private val packageTable: KotlinPackageEntryTable
+) : Comparator<ImportPath> {
+    override fun compare(import1: ImportPath, import2: ImportPath): Int {


I would rewrite it like this:

private val comparator = compareBy<ImportPath>( { import -> bestMatchIndex(import) }, { import -> import.toString() } ) override fun compare(import1: ImportPath, import2: ImportPath): Int = comparator.compare(import1, import2)

IMO it is clearer and leaves less room for error

fedochet · 2020-05-25T07:55:21Z

@gcx11 hi!

I've had to change the sorting implementation a little bit, since it wasn't compatible with the legacy implementation. The problem was that the aliased imports were not sorted according to the order of the entries, and they should (so adding this sorting layout would not change the default import sorting in all projects)

I've decided to amend the fix into your commit and merge it by hand, I hope you don't mind that

Commits from this PR in the master start from 8849a66

Thanks again for your contribution!

vojkny · 2020-05-25T09:05:21Z

@gcx11 hi!

I've had to change the sorting implementation a little bit, since it wasn't compatible with the legacy implementation. The problem was that the aliased imports were not sorted according to the order of the entries, and they should (so adding this sorting layout would not change the default import sorting in all projects)

I've decided to amend the fix into your commit and merge it by hand, I hope you don't mind that

Commits from this PR in the master start from 8849a66

Thanks again for your contribution!

When will this be available? Is there any build we can have this working?

abelkov assigned Alefas Apr 27, 2020

abelkov added the IDEA label Apr 27, 2020

fedochet self-assigned this Apr 28, 2020

fedochet requested changes Apr 29, 2020

View reviewed changes

gcx11 force-pushed the KT-10974 branch from b7764e9 to 92cb6c6 Compare May 1, 2020 20:21

romtsn mentioned this pull request May 2, 2020

Alphabetical import ordering enabled by default is a problem in Android Studio/IntelliJ pinterest/ktlint#527

Closed

gcx11 added 4 commits May 12, 2020 23:08

KT-10974 - add import layout configuration table

5440038

KT-10974 - small fixes

dd0ae9a

KT-10974 - code review changes

617d4b4

KT-10974 - code review changes

a7c3121

gcx11 force-pushed the KT-10974 branch from 92cb6c6 to a7c3121 Compare May 12, 2020 22:35

wb9688 mentioned this pull request May 14, 2020

Add EditorConfig TeamNewPipe/NewPipe#3394

Closed

5 tasks

fedochet closed this May 25, 2020

maybeYouShouldBeTheLawyer mentioned this pull request May 19, 2021

Upgrade gradle to 7.0. square/workflow-kotlin#420

Merged

zach-klippenstein mentioned this pull request May 19, 2021

Review/audit the ignored ktlint rules square/workflow-kotlin#429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KT-10974 Add Code Style: Import Layout Configuration Table #3336

KT-10974 Add Code Style: Import Layout Configuration Table #3336

gcx11 commented Apr 25, 2020

fedochet left a comment

fedochet Apr 28, 2020

fedochet Apr 28, 2020

fedochet Apr 28, 2020

fedochet Apr 28, 2020

fedochet Apr 28, 2020

fedochet Apr 29, 2020

gcx11 May 1, 2020

fedochet Apr 29, 2020

gcx11 May 1, 2020

fedochet May 7, 2020 •

edited

Loading

gcx11 May 16, 2020

fedochet May 17, 2020

fedochet Apr 29, 2020

fedochet Apr 29, 2020

fedochet Apr 29, 2020

fedochet Apr 29, 2020

fedochet commented May 25, 2020

vojkny commented May 25, 2020

KT-10974 Add Code Style: Import Layout Configuration Table #3336

KT-10974 Add Code Style: Import Layout Configuration Table #3336

Conversation

gcx11 commented Apr 25, 2020

fedochet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedochet May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedochet commented May 25, 2020

vojkny commented May 25, 2020

fedochet May 7, 2020 •

edited

Loading