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.

Sign up for GitHub

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

Jump to bottom

Add generated comments to Kotlin files (default true) #486

Merged
merged 1 commit into from
May 23, 2022
Merged

Add generated comments to Kotlin files (default true) #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 0 deletions thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,8 @@ class ThriftyCompiler {
gen.emitBigEnums()
}

gen.emitFileComment(!omitFileComments)

if (kotlinFilePerType) {
gen.filePerType()
} else {
Expand Down
18 changes: 18 additions & 0 deletions thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ import com.squareup.kotlinpoet.jvm.jvmField
import com.squareup.kotlinpoet.jvm.jvmStatic
import com.squareup.kotlinpoet.tag
import okio.ByteString
import java.time.Instant
import java.time.format.DateTimeFormatter
import java.util.Locale

private object Tags {
Expand Down Expand Up @@ -149,6 +151,7 @@ class KotlinCodeGenerator(
private var emitJvmName: Boolean = false
private var emitJvmStatic: Boolean = false
private var emitBigEnums: Boolean = false
private var emitFileComment: Boolean = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I'd prefer this to default to false, if only to minimize the disruption to others. AFAIK most people using thrifty are still manually checking in the generated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjamin-bader I don't have a strong opinion either way, but it does default to true for Java sources, which means the default will change depending on whether you generate Java or Kotlin. I will defer to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You make a good point. I'd forgotten entirely about the Java code-generator, despite directly referring to it. Shows where our focus is, I suppose. Let's keep it on by default, then.

private var failOnUnknownEnumValues: Boolean = true
private var generateServer: Boolean = false

Expand Down Expand Up @@ -270,6 +273,10 @@ class KotlinCodeGenerator(
this.emitBigEnums = true
}

fun emitFileComment(emitFileComment: Boolean): KotlinCodeGenerator = apply {
this.emitFileComment = emitFileComment
}

fun failOnUnknownEnumValues(value: Boolean = true): KotlinCodeGenerator = apply {
this.failOnUnknownEnumValues = value
}
Expand Down Expand Up @@ -2567,6 +2574,10 @@ class KotlinCodeGenerator(
.addMember("%S", fileName)
.build())
}

if (emitFileComment) {
addFileComment(FILE_COMMENT + DATE_FORMATTER.format(Instant.now()))
}
}
}

Expand Down Expand Up @@ -2606,5 +2617,12 @@ class KotlinCodeGenerator(
private fun String.capitalize(): String {
return this.replaceFirstChar { if (it.isLowerCase()) it.titlecase(Locale.US) else it.toString() }
}

companion object {
private const val FILE_COMMENT =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how you feel about this copypasta; there wasn't an obvious shared place to put the constants but I'm happy to move.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd have imagined that after working in this codebase, you'd have seen enough copypasta from me not to worry about it 😆

(This is absolutely fine by me, thank you for asking)

"Automatically generated by the Thrifty compiler; do not edit!\nGenerated on: "

private val DATE_FORMATTER = DateTimeFormatter.ISO_INSTANT
}
}

26 changes: 24 additions & 2 deletions thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,10 @@ class KotlinCodeGeneratorTest {
|const map<i32, list<string>> Maps = {1: [], 2: ["foo"]}
""".trimMargin()

val text = generate(thrift) { mapClassName("android.support.v4.util.ArrayMap") }
val text = generate(thrift) {
mapClassName("android.support.v4.util.ArrayMap")
emitFileComment(false)
}
.single()
.toString()

Expand Down Expand Up @@ -427,6 +430,7 @@ class KotlinCodeGeneratorTest {
val file = generate(thrift) {
emitJvmName()
filePerNamespace()
emitFileComment(false)
}
.single()
file.shouldCompile()
Expand Down Expand Up @@ -455,6 +459,7 @@ class KotlinCodeGeneratorTest {
val file = generate(thrift) {
emitJvmName()
filePerType()
emitFileComment(false)
}
.single()
file.shouldCompile()
Expand Down Expand Up @@ -1346,8 +1351,25 @@ class KotlinCodeGeneratorTest {
file.shouldCompile()
}

@Test
fun `Files should add generated comment`() {
val thrift = """
|namespace kt test.comment
|
|const i32 FooNum = 42
""".trimMargin()

val file = generate(thrift) { emitFileComment(true) }
.single()
file.shouldCompile()

val lines = file.toString().split("\n")
lines[0] shouldBe "// Automatically generated by the Thrifty compiler; do not edit!"
lines[1] shouldContain "\\/\\/ Generated on: [0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}\\.\\S+".toRegex()
}

private fun generate(thrift: String, config: (KotlinCodeGenerator.() -> KotlinCodeGenerator)? = null): List<FileSpec> {
val configOrDefault = config ?: { this }
val configOrDefault = config ?: { emitFileComment(false) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also default this to true but it's a bit annoying to handle the changing timestamp.

return KotlinCodeGenerator()
.run(configOrDefault)
.generate(load(thrift))
Expand Down