search

Kotlin / binary-compatibility-validator

Public API management tool
Apache License 2.0
761 stars 55 forks source link

Eliminate trailing double newline in dump #192

Open JakeWharton opened 4 months ago

JakeWharton commented 4 months ago

Closes #133

fzhinkin commented 4 months ago

@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.

JakeWharton commented 4 months ago

Sure, no rush. Thanks. I'll do a rebase in the mean time later today.

fzhinkin commented 3 months ago

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 KotlinApiCompareTask has to be update to ignore trailing newlines. I would also add a dump overload accepting a parameter controlling trailing newlines and preserve the current behavior for the old overload, so that existing dump users (like Kotlin stdlib) won't experience any changes in a dump format:

public fun <T : Appendable> List<ClassBinarySignature>.dump(to: T): T = dumpTo(to, true)
public fun <T : Appendable> List<ClassBinarySignature>.dump(to: T, addTrailingNewline: Boolean): T { .. }
JakeWharton commented 3 months ago

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.

fzhinkin commented 3 months ago

The format has changed before in minor version bumps.

That's true, but it was usually associated with improvements in public ABI handling, not just the format itself.

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.

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 KotlinApiCompareTask's behavior and make everything unnoticeable (for those who don't update the dump, ofc).