8000 Fix #4037: Format sources with scalafmt by MaximeKjaer · Pull Request #4260 · scala-js/scala-js · GitHub
[go: up one dir, main page]
Skip to content

Fix #4037: Format sources with scalafmt #4260

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix #4037: Format sources with scalafmt #4260

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45a6fa0
Add scalafmt
MaximeKjaer Oct 29, 2020
09ea065
Fix invalid Scaladoc comment
MaximeKjaer Oct 12, 2020
8000 0e1a937
Format sources with scalafmt
MaximeKjaer Nov 3, 2020
4359def
Add .git-blame-ignore-revs
MaximeKjaer Nov 3, 2020
4ce36db
Add scalafmt formatting check to CI
MaximeKjaer Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add scalafmt 
Add scalafmt sbt commands.

Remove coding style rules deprecated by scalafmt, and scalastyle rules
overlapping with scalafmt.
  • Loading branch information
@MaximeKjaer
MaximeKjaer committed Nov 3, 2020
commit 45a6fa02e168cdd93b15cae98334861ec13a41d3
7 changes: 7 additions & 0 deletions .scalafmt.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
version = 2.7.4
preset = Scala.js
maxColumn = 100
newlines.alwaysBeforeMultilineDef = false
rewrite.rules = [AvoidInfix, PreferCurlyFors]
docstrings.style = AsteriskSpace
danglingParentheses.ctrlSite = false
46 changes: 3 additions & 43 deletions CODINGSTYLE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
The Scala.js project has a strict policy regarding coding style.
This is one of the cornerstones that has allowed Scala.js to maintain a clean, consistent and maintainable codebase.

This document tries to document the style in use as much as possible to make it easier for everyone to contribute.
Most coding style rules can be applied automatically by scalafmt. It is recommended that you configure your editor to reformat on save; see the [scalafmt instructions](https://scalameta.org/scalafmt/docs/installation.html) for how to configure this.

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

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 codebase breaks 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 there any 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

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 of https://scalameta.org/scalafmt/docs/configuration.html#newlinessource?


The Scala.js core team has decided to designate a single developer to drive and "maintain" the project's coding style.
This allows us to maintain a consistent, yet flexible style.
Expand All @@ -32,10 +33,6 @@ In some cases, if breaking a line makes it significantly less readable, it can g
Rationale: when reviewing on GitHub, only 120 characters are visible; when reviewing on a mobile phone, only 80 characters are visible.
And we do review on mobile phone quite a lot.

#### Where to break a line

A line can be broken after either a `,` or a `(`, or possibly after a binary operator in a long expression.

### Indentation

In general, indentation is 2 spaces, except continuation lines, which are indented 4 spaces.
Expand Down Expand Up @@ -113,26 +110,6 @@ def abs(x: Int): Int =
-x
```

#### Long expressions with binary operators

Very long expressions consisting of binary operators at their "top-level" can be broken *without indentation* if they are alone in their brace-delimited block or their actual parameter.
This happens mostly for long chains of `&&`s, `||`s, or string concatenations.
Here is an example:

```scala
val isValidIdent = {
ident != "" &&
ident.charAt(0).isUnicodeIdentifierStart &&
ident.tail.forall(_.isUnicodeIdentifierPart)
}

if (!isValidIdent) {
reportError(
"This string is very long and will " +
"span several lines.")
}
```

#### Braces in lambdas

In lambdas (anonymous functions), the opening brace must be placed before the formal arguments, and not after the `=>`:
Expand Down Expand Up @@ -160,21 +137,6 @@ val f = (x: Int) => body
val ys = xs.map(x => x + 1)
```

### Spaces

There must not be any space before the following tokens: `:` `,` `;` `)`

There must be exactly one space after the following tokens: `:` `,` `;` `if` `for` `while`

There must be exactly one space before the tokens `=` and `=>`, and either exactly one space or a new line after them.
Exception: `=>` may be vertically aligned instead in some scenarios: see [the "Pattern matching" section](#pattern-matching).

There must be exactly one space before and after `{` and `}`.
With the exception of partial import, where there is no space on either side.

Binary operators must have a single space on both sides.
Unary operators must not be followed by a space.

### Method call style

Usually, parentheses should be used for actual parameters to a method call.
Expand Down Expand Up @@ -214,8 +176,6 @@ Procedure syntax must not be used.
Side-effect-free methods without formal parameters should be declared without `()`, unless either a) it overrides a method defined with `()` (such as `toString()`) or b) it implements a Java method in the Java libraries.

The signature of a method is technically a single line, and hence, if it has to be broken due to line length reasons, subsequent lines should be indented 4 spaces.
As a reminder, the line can be broken right after a `,` or a `(` (and not, for example, after `implicit` or `:`).
You should avoid breaking the line between the last parameter and the result type; going over the 80 characters limit is preferred in that case.

### `for` comprehensions

Expand Down
2 changes: 2 additions & 0 deletions linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/CoreJSLib.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ private[emitter] object CoreJSLib {
buf += extractWithGlobals(globalVarDef("linkingInfo", CoreVar, linkingInfo))
}

// format: off
private def defineJSBuiltinsSnapshotsAndPolyfills(): Unit = {
def genPolyfillFor(builtinName: String): Tree = builtinName match {
case "is" =>
Expand Down Expand Up @@ -368,6 +369,7 @@ private[emitter] object CoreJSLib {
SymbolRef, genPolyfillFor("privateJSFieldSymbol"))))
}
}
// format: on

private def declareCachedL0(): Unit = {
if (!allowBigIntsForLongs)
Expand Down
16 changes: 14 additions & 2 deletions project/Build.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import org.scalajs.jsenv.nodejs.NodeJSEnv

import ScalaJSPlugin.autoImport.{ModuleKind => _, _}
import org.scalastyle.sbt.ScalastylePlugin.autoImport.scalastyle
import org.scalafmt.sbt.ScalafmtPlugin.autoImport._
import org.scalafmt.sbt.ConcurrentRestrictionTags
import ExternalCompile.scalaJSExternalCompileSettings
import Loggers._

Expand Down Expand Up @@ -592,7 +594,9 @@ object Build {

val keys = Seq[TaskKey[_]](
clean, headerCreate in Compile, headerCreate in Test,
headerCheck in Compile, headerCheck in Test, scalastyleCheck
headerCheck in Compile, headerCheck in Test, scalastyleCheck,
scalafmt in Compile, scalafmt in Test, scalafmtCheck in Compile, scalafmtCheck in Test,
scalafmtAll, scalafmtCheckAll
)

for (key <- keys) yield {
Expand All @@ -606,6 +610,8 @@ object Build {
}
},

concurrentRestrictions += Tags.limit(org.scalafmt.sbt.ConcurrentRestrictionTags.Scalafmt, 1),

headerCreate := (headerCreate in Test).dependsOn(headerCreate in Compile).value,
headerCheck := (headerCheck in Test).dependsOn(headerCheck in Compile).value,

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

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

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 the partest 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

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 a val ignoreCodeStyleSettings or something like this.

scalafmt in Compile := {},
scalafmt in Test := {},
scalafmtCheck in Compile := false,
scalafmtCheck in Test := false,
scalafmtAll := {},
scalafmtCheckAll := {},

// The Scala lib is full of warnings we don't want to see
scalacOptions ~= (_.filterNot(
Expand Down
2 changes: 2 additions & 0 deletions project/build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ addSbtPlugin("org.scalastyle" % "scalastyle-sbt-plugin" % "1.0.0")

addSbtPlugin("org.portable-scala" % "sbt-platform-deps" % "1.0.0")

addSbtPlugin("org.scalameta" % "sbt-scalafmt" % "2.4.0")

libraryDependencies += "com.google.jimfs" % "jimfs" % "1.1"

libraryDependencies += "org.eclipse.jgit" % "org.eclipse.jgit.pgm" % "3.2.0.201312181205-r"
Expand Down
37 changes: 0 additions & 37 deletions scalastyle-config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,6 @@
<scalastyle>
<name>Scalastyle configuration for Scala.js</name>

<check level="error" enabled="true" class="org.scalastyle.file.FileTabChecker"/>
<check level="error" enabled="true" class="org.scalastyle.file.WhitespaceEndOfLineChecker"/>
<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

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
Contributor Author

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 by scalafmtCheckAll.


<!--
Enforce the hard limit on line length, which is 120 characters.
Most of the time the soft limit of 80 characters should be observed, but
we can hardly tell an automated style checker about a soft limit.
-->
<check level="error" enabled="true" class="org.scalastyle.file.FileLineLengthChecker">
<parameters>
<parameter name="maxLineLength"><![CDATA[120]]></parameter>
</parameters>
</check>

<check level="error" enabled="true" class="org.scalastyle.scalariform.RedundantIfChecker"/>
<check level="error" enabled="true" class="org.scalastyle.scalariform.SimplifyBooleanExpressionChecker"/>

Expand Down Expand Up @@ -59,23 +41,4 @@
<check level="error" enabled="false" class="org.scalastyle.scalariform.ForBraceChecker"/>

<check level="error" enabled="true" class="org.scalastyle.scalariform.PublicMethodsHaveTypeChecker"/>

<check level="error" enabled="true" class="org.scalastyle.scalariform.NoWhitespaceBeforeLeftBracketChecker"/>
<check level="error" enabled="true" class="org.scalastyle.scalariform.NoWhitespaceAfterLeftBracketChecker"/>

<check level="error" enabled="true" class="org.scalastyle.scalariform.DisallowSpaceBeforeTokenChecker">
<parameters>
<parameter name="tokens">COLON, COMMA, RPAREN</parameter>
</parameters>
</check>
<check level="error" enabled="true" class="org.scalastyle.scalariform.DisallowSpaceAfterTokenChecker">
<parameters>
<parameter name="tokens">LPAREN</parameter>
</parameters>
</check>
<check level="error" enabled="true" class="org.scalastyle.scalariform.EnsureSingleSpaceAfterTokenChecker">
<parameters>
<parameter name="tokens">IF, FOR, WHILE, DO, TRY</parameter>
</parameters>
</check>
</scalastyle>
0