Skip to content
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

fix: update to latest kotlin-editor and don't delete complex statements in dependencies blocks. #126

Merged
merged 1 commit into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ dagp = "1.30.0"
java = "11"
junit5 = "5.7.2"
kotlin = "1.9.24"
kotlinEditor = "0.8"
kotlinEditor = "0.9"
mavenPublish = "0.28.0"
moshi = "1.14.0"
retrofit = "2.9.0"
Expand Down
46 changes: 32 additions & 14 deletions sort/src/main/kotlin/com/squareup/sort/kotlin/KotlinSorter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ import cash.grammar.kotlindsl.model.gradle.DependencyContainer
import cash.grammar.kotlindsl.parse.Parser
import cash.grammar.kotlindsl.utils.Blocks.isDependencies
import cash.grammar.kotlindsl.utils.CollectingErrorListener
import cash.grammar.kotlindsl.utils.Context.fullText
import cash.grammar.kotlindsl.utils.DependencyExtractor
import cash.grammar.kotlindsl.utils.Whitespace
import com.squareup.cash.grammar.KotlinParser.NamedBlockContext
import com.squareup.cash.grammar.KotlinParser.StatementContext
import com.squareup.cash.grammar.KotlinParserBaseListener
import com.squareup.parse.AlreadyOrderedException
import com.squareup.parse.BuildScriptParseException
Expand All @@ -18,7 +20,6 @@ import org.antlr.v4.runtime.CharStream
import org.antlr.v4.runtime.CommonTokenStream
import org.antlr.v4.runtime.TokenStreamRewriter
import java.nio.file.Path
import kotlin.io.path.absolutePathString

public class KotlinSorter private constructor(
private val input: CharStream,
Expand All @@ -36,10 +37,7 @@ public class KotlinSorter private constructor(
)

private val dependencyComparator = DependencyComparator()
private val mutableDependencies = MutableDependencies(
mutableMapOf(),
mutableListOf()
)
private val mutableDependencies = MutableDependencies()
private val ordering = Ordering()

private var level = 0
Expand Down Expand Up @@ -99,7 +97,8 @@ public class KotlinSorter private constructor(

private fun collectDependencies(container: DependencyContainer) {
val declarations = container.getDependencyDeclarations().map { KotlinDependencyDeclaration(it) }
mutableDependencies.nonDeclarations += container.getNonDeclarations()
mutableDependencies.expressions += container.getExpressions()
mutableDependencies.statements += container.getStatements()

ordering.collectDependencies(declarations)

Expand All @@ -119,10 +118,26 @@ public class KotlinSorter private constructor(

appendLine("dependencies {")

// not-easily-modelable elements
mutableDependencies.nonDeclarations.forEach {
/*
* not-easily-modelable elements
*/

// An example of a statement, in this context, is an if-expression or property expression (declaration)
mutableDependencies.statements.forEach { stmt ->
append(indent.repeat(level))
appendLine(stmt.fullText(input)!!)

didWrite = true
}

if (didWrite && mutableDependencies.expressions.isNotEmpty()) {
appendLine()
}

// An example of an expression, in this context, is a function call like `add("extraImplementation", "foo")`
mutableDependencies.expressions.forEach { expr ->
append(indent.repeat(level))
appendLine(it)
appendLine(expr)

didWrite = true
}
Expand All @@ -131,8 +146,9 @@ public class KotlinSorter private constructor(
appendLine()
}

// declarations
mutableDependencies.declarations().sortedWith(KotlinConfigurationComparator)
// straightforward declarations
mutableDependencies.declarations()
.sortedWith(KotlinConfigurationComparator)
.forEachIndexed { i, entry ->
if (i != 0) appendLine()

Expand Down Expand Up @@ -185,15 +201,17 @@ public class KotlinSorter private constructor(
}

private class MutableDependencies(
val dependenciesByConfiguration: MutableMap<String, MutableList<KotlinDependencyDeclaration>>,
val nonDeclarations: MutableList<String>,
val dependenciesByConfiguration: MutableMap<String, MutableList<KotlinDependencyDeclaration>> = mutableMapOf(),
val expressions: MutableList<String> = mutableListOf(),
val statements: MutableList<StatementContext> = mutableListOf(),
) {

fun declarations() = dependenciesByConfiguration.entries

fun clear() {
dependenciesByConfiguration.clear()
nonDeclarations.clear()
expressions.clear()
statements.clear()
}
}

Expand Down
42 changes: 42 additions & 0 deletions sort/src/test/groovy/com/squareup/sort/GroovySorterSpec.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package com.squareup.sort
import com.squareup.parse.AlreadyOrderedException
import com.squareup.parse.BuildScriptParseException
import com.squareup.sort.groovy.GroovySorter
import com.squareup.sort.kotlin.KotlinSorter
import spock.lang.PendingFeature
import spock.lang.Specification
import spock.lang.TempDir

Expand Down Expand Up @@ -180,6 +182,46 @@ final class GroovySorterSpec extends Specification {
)
}

// GroovySorter may never support complex syntax in a dependencies block
@PendingFeature
def "doesn't remove complex statements when sorting"() {
given:
def buildScript = dir.resolve('build.gradle.kts')
Files.writeString(buildScript,
'''\
dependencies {
implementation(libs.c)
api(libs.d)
testImplementation("g:e:1")

if (org.apache.tools.ant.taskdefs.condition.Os.isArch("aarch64")) {
// Multi-line comment about why we're
// doing this.
testImplementation("io.github.ganadist.sqlite4java:libsqlite4java-osx-aarch64:1.0.392")
}
}'''.stripIndent()
)
def sorter = KotlinSorter.of(buildScript)

expect:
assertThat(sorter.rewritten()).isEqualTo(
'''\
dependencies {
api(libs.d)

implementation(libs.c)

testImplementation("g:e:1")

if (org.apache.tools.ant.taskdefs.condition.Os.isArch("aarch64")) {
// Multi-line comment about why we're
// doing this.
testImplementation("io.github.ganadist.sqlite4java:libsqlite4java-osx-aarch64:1.0.392")
}
}'''.stripIndent()
)
}

def "can sort build script with four-space tabs"() {
given:
def buildScript = dir.resolve('build.gradle')
Expand Down
46 changes: 46 additions & 0 deletions sort/src/test/groovy/com/squareup/sort/KotlinSorterSpec.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,52 @@ class KotlinSorterSpec extends Specification {
)
}

def "doesn't remove complex statements when sorting"() {
given:
def buildScript = dir.resolve('build.gradle.kts')
Files.writeString(buildScript,
'''\
dependencies {
val complex = "a:complex:$expression"

implementation(complex)
api(libs.d)
testImplementation("g:e:1")

add("extraImplementation", libs.a)

if (org.apache.tools.ant.taskdefs.condition.Os.isArch("aarch64")) {
// Multi-line comment about why we're
// doing this.
testImplementation("io.github.ganadist.sqlite4java:libsqlite4java-osx-aarch64:1.0.392")
}
}'''.stripIndent()
)
def sorter = KotlinSorter.of(buildScript)

expect:
assertThat(sorter.rewritten()).isEqualTo(
'''\
dependencies {
val complex = "a:complex:$expression"

if (org.apache.tools.ant.taskdefs.condition.Os.isArch("aarch64")) {
// Multi-line comment about why we're
// doing this.
testImplementation("io.github.ganadist.sqlite4java:libsqlite4java-osx-aarch64:1.0.392")
}

add("extraImplementation", libs.a)

api(libs.d)

implementation(complex)

testImplementation("g:e:1")
Comment on lines +244 to +258
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this ordering could be improved further. We can probably specially parse the add(...) expression, and maybe also ensure that property expressions are modeled as such and always at top, rather than at top by accident. The if-expression is more complex, and arguably an anti-pattern, so I think we'll just leave that alone.

}'''.stripIndent()
)
}

def "can sort build script with four-space tabs"() {
given:
def buildScript = dir.resolve('build.gradle.kts')
Expand Down
Loading