-
Notifications
You must be signed in to change notification settings - Fork 59
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
Eliminate trailing double newline in dump #192
base: master
Are you sure you want to change the base?
Eliminate trailing double newline in dump #192
Conversation
@@ -27,8 +27,10 @@ internal fun BaseKotlinGradleTest.test( | |||
createNewFile() | |||
} | |||
|
|||
scope.files.forEach { | |||
scope.files.forEachIndexed { index, 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.
The fact that you can add multiple dump files and have them magically joined is weird. It's only used in one test. That test should specify a combined file and this functionality should be eliminated.
Can do in a follow-up if you agree.
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.
It's not only about dumps, and I personally find it quite convenient when it comes to constructing Gradle build scripts, like here:
binary-compatibility-validator/src/functionalTest/kotlin/kotlinx/validation/test/InputJarTest.kt
Line 18 in 218728f
resolve("/examples/gradle/configuration/jarAsInput/inputJar.gradle.kts") |
@@ -34,8 +33,3 @@ private fun assertEqualsToFile(expectedFile: File, actual: CharSequence) { | |||
|
|||
assertEquals(expectedText, actualText, "Actual data differs from file content: ${expectedFile.name}\nTo overwrite the expected API rerun with -Doverwrite.output=true parameter\n") | |||
} | |||
|
|||
private fun CharSequence.trimTrailingWhitespacesAndAddNewlineAtEOF(): String = |
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.
Another testing oddity. This fundamentally undermines the ability to validate the textual representation of the dump and prevented actually asserting against the behavior change in this PR.
The presence or absence of trailing whitespace on a line and the presence or absence of trailing newlines are part of the output and should be validated as such.
@JakeWharton thanks for addressing the issue. Sorry for the long time it's taking me to review it, I'll look at it this week. |
Sure, no rush. Thanks. I'll do a rebase in the mean time later today. |
I'm not 100% sure if a double newline at the end of a dump was intentional (@ilya-g correct me if it was), but changing it right now will annoy a lot of users as their dumps will no longer pass validation. So, at least
|
The format has changed before in minor version bumps. Since the next version is a minor version bump, perhaps we can instead warn if the double trailing newline is present. And then remove that for 0.16 where we only accept the new format. |
That's true, but it was usually associated with improvements in public ABI handling, not just the format itself.
The fix is about cleaning up the mess BCV made previously itself, not something users deliberately made themselves. So I don't think we should bother users if there's an option to slightly change |
Closes #133