MINOR; Update scalafmt to latest version #12475

mdedetrich · 2022-08-02T20:36:27Z

Updates scalafmt to the latest version. This not only means migrating
the checkstyle/.scalafmt.conf file but updating spotless as well. Due
to improvements in the new version of scalafmt, it now also
alphabetically sorts imports.

The diff to the actual .scala source files is due to the alphabetical sorting of imports. I verified that this PR passes spotless/scalacheck by running ./gradlew :spotlessScalaCheck locally.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

mdedetrich · 2022-08-02T20:37:20Z

build.gradle

@@ -30,7 +30,7 @@ buildscript {
 }

 plugins {
-  id 'com.diffplug.spotless' version '5.12.5'
+  id 'com.diffplug.spotless' version '6.9.0'


This version needed to be updated otherwise spotless would bring resolve the wrong Scala version for scalafmt.

mdedetrich · 2022-08-02T20:42:23Z

checkstyle/.scalafmt.conf

+version = 3.5.8
+runner.dialect = scala213
+docstrings.style = Asterisk
+docstrings.wrap = false


This forces scalafmt to ignore docstrings when formatting code. The new version of scalafmt works on docstrings however it doesn't support manual html formatting which is done in kafka scala streams.

mdedetrich · 2022-08-03T06:41:19Z

@divijvaidya This is one of the first PR's in adding the linting/formatting using scalafmt/scalafix for the Scala sources within Kafka that you talked about here #12471 (comment). @cadonna Do you want to have a look as well since its part of kafka scala streams?

divijvaidya

Thanks for making this change.

Note that build is failing related to changes in this PR. We would probably need to fix the error but I think I might have already fixed it in MINOR: Catch InvocationTargetException explicitly and propagate underlying cause #12230 If that lands, then the check should pass here.

[2022-08-02T22:58:41.580Z] * What went wrong:

[2022-08-02T22:58:41.580Z] Execution failed for task ':spotlessScala'.

[2022-08-02T22:58:41.580Z] > java.lang.reflect.InvocationTargetException

I have verified that the style guide we follow for this project is not prescriptive about formatting of the docs. Hence, your change should be ok.

divijvaidya · 2022-08-03T09:10:37Z

checkstyle/.scalafmt.conf

@@ -12,7 +12,10 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
-docstrings = JavaDoc
+version = 3.5.8
+runner.dialect = scala213


We still support scala 2.12 in this project.

By specifying the dialect as 2.13, would we fail where syntax for 2.12 is used? If yes, are there any such cases in the project? (The build should tell us that after the existing spotless check passes).

The Scala 2.13 dialect is backwards compatible with 2.12. I have applied the dialect when updating from scalafmt 2 to scalafmt 3 with various popular OSS Scala projects and there isn't any issues (in any case the Scala code that is used im Kafka is quite conservative)

I have verified that the style guide we follow for this project is not prescriptive about formatting of the docs. Hence, your change should be ok.

Do you think it makes sense to format the docs or should I leave the PR as is?

Note that build is failing related to changes in this PR. We would probably need to fix the error but I think I might have already fixed it in MINOR: Catch InvocationTargetException explicitly and propagate underlying cause #12230 If that lands, then the check should pass here.

In that case it's sensible to wait for your PR to land and then I can rebase.

Do you think it makes sense to format the docs or should I leave the PR as is?

I think we should leave the PR as is. Converting the docs in the entire project will be cumbersome without any benefit.

In that case it's sensible to wait for your PR to land and then I can rebase.

Thanks, in that case, would you kindly review that PR please if you get some time? It's been pending for a while now waiting to get some attention.

Thanks, in that case, would you kindly review that PR please if you get some time? It's been pending for a while now waiting to get some attention.

I just landed a review of the PR.

ijuma · 2022-08-10T13:50:43Z

The scala 2.12 build failed https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12475/

mdedetrich · 2022-08-10T19:07:11Z

@ijuma Thanks for the pickup.

I have investigated the root cause of the problem, basically when you specify the scala version with -PscalaVersion=2.12 it also happens to override the Scala version that is used by scalafmt and the latest version of scalafmt (3.5.8) only runs with Scala 2.13.x. This is normally a non issue because scalafmt is designed to be used like a cli tool, i.e. its meant to be run independently of your project.

I created an issue upstream at diffplug/spotless#1273

mdedetrich · 2022-08-21T12:44:45Z

@ijuma So I have added a commit which adds a workaround for the Scala runtime conflict issues. The CI is failing but it appears it's not using the updated Jenkinsfile in the commit so I suspect that after a merge the Apache CI should use the updated Jenkinsfile?

@mimaison For visibility as well.

mdedetrich · 2022-08-22T12:02:13Z

Actually lets wait on merging this PR, it may be possible to fix the underlying problem in spotless (see diffplug/spotless#1273 (comment))

mdedetrich · 2022-08-23T23:20:32Z

@ijuma So I have some good news, I managed to completely the solve the underlying problem upstream (see diffplug/spotless#1273 (comment)). The PR has been updated/rebased with the newest spotless and as you can see spotlessScalaCheck even works when Scala 2.12 is set, i.e.

./gradlew -PscalaVersion=2.12 :spotlessScalaCheck

Also tagging @mimaison for visibility

mdedetrich · 2022-08-29T12:33:39Z

Tagging @C0urante for visibility

C0urante

Thanks Matthew! One small question about the changes.

I should note that I wasn't a fan of the import shuffling initially, but it does seem like we wanted import sorting when we originally introduced Scala formatting in #4965 and included the SortImports rule. So although someone else more familiar with this part of the code base may feel differently, at least from my perspective these changes are fine.

I've kicked off another Jenkins run to see if we can get at least one green build with Scala 2.12.

checkstyle/.scalafmt.conf

Updates scalafmt to the latest version. This not only means migrating the checkstyle/.scalafmt.conf file but updating spotless as well. Due to improvements in the new version of scalafmt, it now also alphabetically sorts imports.

mdedetrich · 2022-08-30T07:02:42Z

I've kicked off another Jenkins run to see if we can get at least one green build with Scala 2.12.

So Jenkins did in fact pass this PR as green build. I have just pushed to this PR so another build has been triggered but https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-12475/8/pipeline is the build that passed.

C0urante

LGTM! I'd like it if someone more familiar with Streams could also take a look before merging, though.

C0urante · 2022-08-30T11:59:22Z

@vvcephei @cadonna would either of you have time to double-check this quick Scalafmt bump?

mdedetrich commented Aug 2, 2022

View reviewed changes

mdedetrich force-pushed the update-scalafmt branch from 3e7c51f to 9687494 Compare August 2, 2022 20:39

mdedetrich commented Aug 2, 2022

View reviewed changes

divijvaidya reviewed Aug 3, 2022

View reviewed changes

mdedetrich mentioned this pull request Aug 10, 2022

Force scalaVersion when using scala scalafmt extension diffplug/spotless#1273

Closed

mdedetrich mentioned this pull request Aug 11, 2022

gradle-scala plugin overrides isolated gradle provisioner scala version in spotless gradle/gradle#21502

Closed

mdedetrich force-pushed the update-scalafmt branch 2 times, most recently from a4e4dd9 to a949f92 Compare August 21, 2022 12:24

mdedetrich force-pushed the update-scalafmt branch from a949f92 to 938aea6 Compare August 21, 2022 12:46

mdedetrich force-pushed the update-scalafmt branch from 938aea6 to db674ed Compare August 23, 2022 22:48

mdedetrich force-pushed the update-scalafmt branch from db674ed to 553e87b Compare August 24, 2022 13:38

C0urante reviewed Aug 29, 2022

View reviewed changes

checkstyle/.scalafmt.conf Show resolved Hide resolved

MINOR; Update scalafmt to latest version

8578153

Updates scalafmt to the latest version. This not only means migrating the checkstyle/.scalafmt.conf file but updating spotless as well. Due to improvements in the new version of scalafmt, it now also alphabetically sorts imports.

mdedetrich force-pushed the update-scalafmt branch from 553e87b to 8578153 Compare August 30, 2022 06:58

C0urante approved these changes Aug 30, 2022

View reviewed changes

C0urante merged commit e138772 into apache:trunk Sep 12, 2022

mdedetrich deleted the update-scalafmt branch September 13, 2022 21:07

MINOR; Update scalafmt to latest version #12475

MINOR; Update scalafmt to latest version #12475

Uh oh!

Conversation

mdedetrich commented Aug 2, 2022

Committer Checklist (excluded from commit message)

Uh oh!

mdedetrich Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

mdedetrich Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

mdedetrich commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divijvaidya left a comment

Choose a reason for hiding this comment

Uh oh!

divijvaidya Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

mdedetrich Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divijvaidya Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdedetrich Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

ijuma commented Aug 10, 2022

Uh oh!

mdedetrich commented Aug 10, 2022

Uh oh!

mdedetrich commented Aug 21, 2022

Uh oh!

mdedetrich commented Aug 22, 2022

Uh oh!

mdedetrich commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdedetrich commented Aug 29, 2022

Uh oh!

C0urante left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdedetrich commented Aug 30, 2022

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

C0urante commented Aug 30, 2022

Uh oh!

Uh oh!

mdedetrich commented Aug 3, 2022 •

edited

Loading

mdedetrich Aug 3, 2022 •

edited

Loading

divijvaidya Aug 3, 2022 •

edited

Loading

mdedetrich commented Aug 23, 2022 •

edited

Loading

C0urante left a comment •

edited

Loading