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

Upgrade Ktlint maven coordinate (com.pinterest), default version (0.32.0) #394

Merged
merged 7 commits into from
Apr 24, 2019
Merged

Upgrade Ktlint maven coordinate (com.pinterest), default version (0.32.0) #394

merged 7 commits into from
Apr 24, 2019

Conversation

sharedprophet
Copy link

Update ktlint coords.

@sharedprophet sharedprophet marked this pull request as ready for review April 23, 2019 18:51
@sharedprophet sharedprophet changed the title Ktlint maven coordinate Upgrade Ktlint maven coordinate (com.pinterest), default version (0.32.0) Apr 23, 2019
@nedtwigg
Copy link
Member

Is com.pinterest:ktlint the new official ktlint? What version is the first pinterest version? We care about backwards compatibility, so we should do a comparison, if version > blah then group ="com.pinterest" else group = "com.github.shyiko".

@JLLeitschuh
Copy link
Member

Here's how we resolved it for Ktlint-Gradle.

JLLeitschuh/ktlint-gradle#228

Since Ktlint implements SemVer for all versioning, we imported a library that supports SemVer comparisons to handle all this logic.

https://github.com/swiftzer/semver

@nedtwigg I don't know if you have an existing methodology for doing SemVer comparisons. Don't know what your thoughts are on adding a library to handle this sort of thing.

@sharedprophet
Copy link
Author

0.31.0 is the last of com.github.shyiko, 0.32.0 is the first for com.pinterest

https://mvnrepository.com/artifact/com.github.shyiko/ktlint
https://mvnrepository.com/artifact/com.pinterest/ktlint

@nedtwigg
Copy link
Member

what your thoughts are on adding a library to handle this sort of thing.

I'm against adding a dependency. Elsewhere in the code where we've had to change behavior based on a version, we've done feature-testing rather than version-testing, but that won't work here.

Every shyiko version has form 0.x.x, So if you did a regex like 0\.([0-9]+)\.([0-9]+) and then compared the digit to 31, that would work.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Thanks @sharedprophet! This looks great, I've got a minor nit on the implementation.

Only thing we need to merge is a test - need to make sure it works for 0.32.0 and 0.31.0. Here's the existing test:

@Test
public void behavior() throws Exception {
// Must use jcenter because `com.andreapivetta.kolor:kolor:0.0.2` isn't available on mavenCentral.
// It is a dependency of ktlint.
FormatterStep step = KtLintStep.create(TestProvisioner.jcenter());
StepHarness.forStep(step)
.testResource("kotlin/ktlint/basic.dirty", "kotlin/ktlint/basic.clean")
.testException("kotlin/ktlint/unsolvable.dirty", assertion -> {
assertion.isInstanceOf(AssertionError.class);
assertion.hasMessage("Error on line: 1, column: 1\n" +
"Wildcard import");
});
}

Maybe have a worksShyiko test and a worksPinterest test?

@nedtwigg
Copy link
Member

Also, we need to update plugin-maven/CHANGES.md and plugin-gradle/CHANGES.md. The same line that you used in CHANGES.md is fine.

@sharedprophet
Copy link
Author

Also, we need to update plugin-maven/CHANGES.md and plugin-gradle/CHANGES.md. The same line that you used in CHANGES.md is fine.

I did update those... working on putting a test in the core tests now.

@nedtwigg
Copy link
Member

LGTM, thanks! I'll let this breathe for 24 hrs then merge and release tomorrow.

Copy link
Member

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

LGTM! Looking forward to this.

@nedtwigg nedtwigg merged commit c6a4ae7 into diffplug:master Apr 24, 2019
@nedtwigg
Copy link
Member

Published in 3.23.0 / 1.23.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants