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

New maven coordinates for 2.0.0 release of Scalafmt #416

Merged
merged 7 commits into from
Jul 10, 2019

Conversation

nsutcliffe
Copy link
Contributor

Scalafmt now releases under org.scalameta rather than com.geirsson as of 2.0.0. Updating so that spotless will work with scalafmt 2.0.0+ (maintains backwards compatability of scalafmt prior to 2.0.0).

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 for the PR to fix #415! Needs a few tweaks.

plugin-gradle/CHANGES.md Show resolved Hide resolved
@nedtwigg
Copy link
Member

nedtwigg commented Jul 8, 2019

Here is convo for fixing Travis issue. The CI is still being triggered, and you can see it here, but it's not publishing back to GitHub for some reason...

@jbduncan
Copy link
Member

jbduncan commented Jul 8, 2019

Looking at the code in this PR, it occurs to me that we're missing a test that scalafmt 2.0.0+ works as intended. However, I don't have my IDE in front of me at the moment, so it's not easy for me to tell what exactly should be tested and where the test should go.

@nedtwigg What do you think? If you agree, would you mind giving @nsutcliffe some guidance on this? :)

@nedtwigg
Copy link
Member

nedtwigg commented Jul 8, 2019

Ah, great catch @jbduncan! Thanks for the fixes @nsutcliffe, one last change and I think we're good to merge.

Here's the ScalaFmt test: https://github.com/diffplug/spotless/blob/master/testlib/src/test/java/com/diffplug/spotless/scala/ScalaFmtStepTest.java

And here are similar tests for the KtLint step where we made sure that the different groups for the different versions work as expected:

@Test
public void worksShyiko() 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("0.31.0", 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");
});
}
@Test
public void worksPinterest() 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("0.32.0", 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");
});
}

@nedtwigg nedtwigg self-requested a review July 8, 2019 18:50
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.

Add a test

@nsutcliffe
Copy link
Contributor Author

I am aware tests are failing on travis; I know that scalafmt has changes some of the defaults in release 2.0.0; I'll investigate the differences against the changes and make sure they are as a result of changes to the defaults.

@nsutcliffe
Copy link
Contributor Author

nsutcliffe commented Jul 8, 2019

The defaults for align.openParenDefnSite changed to false in 1.6 (see the scalafmt documentation), although I note that they didn't release 1.6, so 2.0.0 is the first chance of us seeing this. I believe this fully explains the difference in the test data, and so have updated with new test data for the 2.0.0 tests.

@nedtwigg
Copy link
Member

nedtwigg commented Jul 8, 2019

Looks great, thanks @nsutcliffe! If you'd like any changes to the credit I just pushed up, lemme know :) Otherwise I'll merge this tomorrow.

@nsutcliffe
Copy link
Contributor Author

That's great, thank you for your prompt assistance :-)

@nedtwigg
Copy link
Member

Released in x.24.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