As discussed in#4037, as of Git 2.23, it is possible to ignore certain commits in agit blame (seethis article). This effectively removes the main argument against automatic formatting, which is that it ruinsgit blame. This PR therefore introduces scalafmt to format all sources.

As the current coding style leaves some room for the code author to use some arbitrary judgment, the tool of course generates a very large diff, as it applies rules uniformly. I would argue that this is perhaps more of a good thing than a bad one.

Nevertheless, I still tried to keep the formatting close to the original rules, e.g. with a line width of 80 characters, 4 space indentation for call site continuations, etc (most of these are encoded in theScala.js scalafmt preset). However, a few of the current coding style rules cannot be maintained when formatting with scalafmt; an incomplete list is below:

scalafmt does not support configuring spacing around: in_: Foo | _: Bar
scalafmt does not supportnewlines.implicitParamListModifierPrefer = before at the same time asbinpack.preset = true, so I had to choose between binpacking and preferring line skips before theimplicit keyword in parameter list rather than after; I prioritized binpacking.

If you see anything really ugly in the formatted source code, please let me know and I will try to see if I can reconfigure scalafmt to deal with it better.

I am also including a few other links to get the discussion started:

GitHub does not yet support.git-blame-ignore-revs on their Web interface, but there is acommunity request for it.
The.git-blame-ignore-revs approach is what is recommended by the (quite popular) Python formatter,black, formigrating coding styles without ruining git blame.
An example of a large project using.git-blame-ignore-revs islocust
Configuring Git with.git-blame-ignore-revs works both on the command line, and in VS Code (with no extra configuration; I am usingGitLens for viewinggit blame in the editor)

I still need to remove sections in the coding style that are no longer true, or no longer relevant because they don't need to be enforced manually anymore. However, I thought I would still open the PR as is now, to get the discussion started on this change.

gzm0 reviewed

Oct 20, 2020

View reviewed changes

junit-runtime/src/main/scala/org/hamcrest/CoreMatchers.scala OutdatedShow resolvedHide resolved

Copy link

He-Pin commentedOct 22, 2020

Will you organize theimports too?

Copy link

ContributorAuthor

MaximeKjaer commentedOct 22, 2020

I could add theSortImports scalafmt rewrite rule, but note that this only sorts imports within curly brackets, not import statements (seescalameta/scalafmt#722). For sorting import statements, we would need to add scalafix, with theOrganizeImports rule. So seeing that another tool is needed to properly sort imports, I think it's best to not let scalafmt format imports.

MaximeKjaer force-pushed thescalafmt branch 3 times, most recently fromb4c6c24 to47ceb13Compare

October 26, 2020 21:29

Copy link

ContributorAuthor

MaximeKjaer commentedOct 27, 2020

@gzm0 gates are now passing

MaximeKjaer commented

Oct 27, 2020

View reviewed changes

project/Build.scala OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

MaximeKjaer commentedOct 27, 2020•
edited
Loading

To-do:

Limit scalafmt parallelism to 1 to avoid race conditions
Addscalafmt andscalafmtCheck top-level commands (in addition toscalafmtAll andscalafmtCheckAll)
Disable scalafmt on scalalib
Rebase so that every commit passes CI
Edit coding style to reflect that scalafmt is the authoritative style guide

MaximeKjaer force-pushed thescalafmt branch from47ceb13 to7fa8592Compare

October 29, 2020 21:55

MaximeKjaer requested a review fromgzm0

October 30, 2020 10:08

gzm0 requested changes

Oct 31, 2020

View reviewed changes

Copy link

Contributor

gzm0 left a comment

Choose a reason for hiding this comment

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

On a high level, three comments:

A bunch of ways to cause less commits.
Comments where it seems that scalafmt / the Scala.js style is buggy.
Some of the reformatting breaks other rules we have. How to fix that?

project/Build.scala

		@@ -1146,8 +1152,14 @@ object Build {
		delambdafySetting,
		noClassFilesSettings,

		// Ignore scalastyle for this project
		// Ignore scalastyle and scalafmt for this project
		scalastyleCheck := {},

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

There is another one of these in thepartest project. I'm unsure whether this is just an artifact of the past. IIUC, the code in there was adopted from Scala, but by now is pretty much independent and patching it with upstream commits is not really something we do.

@sjrd any ideas about this?

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

Also, if we need this in multiple places, we should probably put it in aval ignoreCodeStyleSettings or something like this.

linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/CoreJSLib.scalaShow resolvedHide resolved

scalastyle-config.xml

		<check level="error" enabled="true" class="org.scalastyle.file.NewLineAtEofChecker"/>

		<!--Indentation checker, whilst useful gives false positives on the file headers, so disabled for now-->
		<check level="error" enabled="false" class="org.scalastyle.file.IndentationChecker"/>

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

Is this not the case anymore, that it gives false positives on the headers?

Copy link

ContributorAuthor

MaximeKjaerNov 3, 2020

Choose a reason for hiding this comment

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

I don't know if the false positives are gone or not, but in any case there's no need for scalastyle to check indentation anymore as indentation is now scalafmt's responsibility, and is checked byscalafmtCheckAll.

scalastyle-config.xmlShow resolvedHide resolved

javalib/src/main/scala/java/util/function/DoublePredicate.scala Outdated

		@@ -22,7 +22,8 @@ trait DoublePredicate { self =>
		def and(other: DoublePredicate): DoublePredicate = {
		new DoublePredicate {
		def test(value: Double): Boolean =
		self.test(value) && other.test(value) // the order and short-circuit are by-spec
		self.test(value) && other.test(
		value) // the order and short-circuit are by-spec

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

Ugh seriously :(

Copy link

ContributorAuthor

MaximeKjaerNov 3, 2020

Choose a reason for hiding this comment

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

This particular instance was fixed by increasing the line length limit. But in general, it's possible to fix this kind of issue manually in daily development. The thing about scalafmt is that it may not produce the prettiest results on the first try; its output depends somewhat on the whitespace in your input. For instance, suppose you start with this:

self.test(value)&& other.test(value)// the order and short-circuit are by-spec

scalafmt may reformat to something ugly:

self.test(value)&& other.test(    value)// the order and short-circuit are by-spec

But if you're dissatisfied with this, you can make scalafmt break differently by manually editing to the following:

self.test(value)&& other.test(    value)// the order and short-circuit are by-spec

scalafmt will then reformat to something more correct:

self.test(value)&&   other.test(value)// the order and short-circuit are by-spec

With format-on-save enabled in your IDE, this kind of thing becomes pretty seamless:

Once we've settled on a stable list of config rules, I can manually go over the ugliest places and fix them.

compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala Outdated

		@@ -1073,7 +1071,8 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
		!settings.noForwarders && sym.isStatic && !isImplClass(sym) && {
		// Reject non-top-level objects unless opted in via the appropriate option
		scalaJSOpts.genStaticForwardersForNonTopLevelObjects \|\|
		!sym.name.containsChar('$') // this is the same test that scalac performs
		!sym.name.containsChar(
		'$') // this is the same test that scalac performs

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

These comment moves are unfortunate, not sure we can do anything about them :(

Copy link

ContributorAuthor

MaximeKjaerNov 3, 2020

Choose a reason for hiding this comment

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

For really long lines, there's nothing to be done, no. This example, however, is fixed by increasing max line length.

Copy link

kitbellewMar 27, 2021

Choose a reason for hiding this comment

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

have you triedhttps://scalameta.org/scalafmt/docs/configuration.html#newlinesavoidforsimpleoverflow?

compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala

		val methodIdent = encodeMethodSym(m)
		val originalName = originalNameOfMethod(m)
		val jsParams = m.tpe.params.map(genParamDef(_))
		val resultType = toIRType(m.tpe.resultType)

		js.MethodDef(flags, methodIdent, originalName, jsParams, resultType, Some {
		js.MethodDef(flags, methodIdent, originalName, jsParams, resultType,
		Some {
		genApplyMethod(genLoadModule(moduleClass), m, jsParams.map(_.ref))

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

Another example of less indentation for nested stuff :(

compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala Outdated

		reversedArgs =
		genPrimitiveJSRepeatedParam(arg) reverse_::: reversedArgs
		reversedArgs = genPrimitiveJSRepeatedParam(
		arg) reverse_::: reversedArgs

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

Other example of bad assignment?

Copy link

kitbellewMar 27, 2021

Choose a reason for hiding this comment

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

have you triedhttps://scalameta.org/scalafmt/docs/configuration.html#newlinesafterinfix?

CODINGSTYLE.md


		A few of these rules are checked automatically using Scalastyle, but most of them are too complex to teach to an automated tool.
		While most rules are checked automatically by scalafmt and Scalastyle, but there are also a few rules that are too complex to teach to an automated tool.
		This document tries to document the manually enforced rules as much as possible to make it easier for everyone to contribute.

Copy link

Contributor

gzm0Oct 31, 2020

Choose a reason for hiding this comment

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

I realize now that this is problematic: scalafmt's formatting on the current codebasebreaks certain of the rules even though they were followed before (what I have seen is mostly blocks not being present). IMHO this is quite serious :-/

Is thereany scalafmt mode that is much more aggressive and formats everything in a consistent manner (not maybe the manner we have right now)? IMO that might be a better alternative.

Copy link

kitbellewMar 27, 2021

Choose a reason for hiding this comment

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

have you tried and of the modes ofhttps://scalameta.org/scalafmt/docs/configuration.html#newlinessource?

CODINGSTYLE.mdShow resolvedHide resolved

Copy link

Member

sjrd commentedOct 31, 2020

Last time I seriously looked at scalafmt for Scala.js, I came up with the following .scalafmt.conf:
https://github.com/sjrd/scala-js/blob/scalafmt/.scalafmt.conf
Maybe some of the things in there are worth including now.

Also, maybe this could be a good time to reconsider our 80 characters line limit. We've always had 80 soft limit and 120 hard limit. But scalafmt has no concept of soft limit, and that causes all sorts of ugly things, because it will favor an ugly formatting that stays under 80 over something clean that goes a bit over the limit. Maybe we would have way less ugly formattings if we simply dropped that 80 char limit.

MaximeKjaer added5 commits

November 3, 2020 13:37

Add scalafmt

45a6fa0

Add scalafmt sbt commands.Remove coding style rules deprecated by scalafmt, and scalastyle rulesoverlapping with scalafmt.

Fix invalid Scaladoc comment

09ea065

Format sources with scalafmt

0e1a937

Add .git-blame-ignore-revs

4359def

Add scalafmt formatting check to CI

4ce36db

MaximeKjaer force-pushed thescalafmt branch fromed43e35 to4ce36dbCompare

November 3, 2020 17:35

Copy link

ContributorAuthor

MaximeKjaer commentedNov 3, 2020

Thank you@sjrd, I've rebased with something closer to your config. It's not possible to have both binpacking (which is enabled by default in theScala.js preset) andnewlines.implicitParamListModifierPrefer = before, so I removed the latter from your config.

I also increased the line length limit to 100, which seems to work fine; in the end, most lines are around 80, and a few lines go in the 80-100 range, which nicely follows the spirit of the "soft 80, hard 120" rule. For instance, here's a screenshot of a fairly representative part ofGenJSCode.scala, with the editor ruler set to 80 characters:

Copy link

ContributorAuthor

MaximeKjaer commentedNov 3, 2020

Why did the AppVeyor build break? Checking the source files, not much has changed in the offending files:library/src/main/scala/scala/scalajs/js/package.scala has barely changed (just whitespace in two places), andlibrary/src/main/scala/scala/scalajs/concurrent/QueueExecutionContext.scala hasn't changed at all 😕

Copy link

Member

sjrd commentedNov 3, 2020

Why did the AppVeyor build break?

That failure is very weird. I've relaunched the build just because I can't make sense of it.

gzm0 mentioned this pull request

Mar 31, 2021

Set up Scalafmt and Scalafix and other small improvementsportable-scala/portable-scala-reflect#39

Open

Labels

None yet

5 participants

Movatterモバイル変換

Fix #4037: Format sources with scalafmt#4260

Are you sure you want to change the base?

Fix #4037: Format sources with scalafmt#4260

Uh oh!

Conversation

MaximeKjaer commentedOct 19, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

He-Pin commentedOct 22, 2020

Uh oh!

MaximeKjaer commentedOct 22, 2020

Uh oh!

MaximeKjaer commentedOct 27, 2020

Uh oh!

Uh oh!

MaximeKjaer commentedOct 27, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjrd commentedOct 31, 2020

Uh oh!

MaximeKjaer commentedNov 3, 2020

Uh oh!

MaximeKjaer commentedNov 3, 2020

Uh oh!

sjrd commentedNov 3, 2020

Uh oh!

Uh oh!

MaximeKjaer commentedOct 19, 2020•
edited
Loading

MaximeKjaer commentedOct 27, 2020•
edited
Loading