Fix #4037: Format sources with scalafmt #4260
Conversation
|
Will you organize the imports too? |
|
I could add the |
MaximeKjaer
force-pushed the
MaximeKjaer:scalafmt
branch
3 times, most recently
from
b4c6c24
to
47ceb13
|
@gzm0 gates are now passing |
|
To-do:
|
|
On a high level, three comments:
|
| @@ -1146,8 +1152,14 @@ object Build { | |||
| delambdafySetting, | |||
| noClassFilesSettings, | |||
|
|
|||
| // Ignore scalastyle for this project | |||
| // Ignore scalastyle and scalafmt for this project | |||
| scalastyleCheck := {}, | |||
gzm0
Oct 31, 2020
Contributor
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?
gzm0
Oct 31, 2020
Contributor
Also, if we need this in multiple places, we should probably put it in a val ignoreCodeStyleSettings or something like this.
...shared/src/main/scala/org/scalajs/linker/backend/emitter/CoreJSLib.scala
Show resolved
Hide resolved
| <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"/> |
gzm0
Oct 31, 2020
Contributor
Is this not the case anymore, that it gives false positives on the headers?
MaximeKjaer
Nov 3, 2020
Author
Contributor
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.
| @@ -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 | |||
MaximeKjaer
Nov 3, 2020
Author
Contributor
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-specscalafmt may reformat to something ugly:
self.test(value) && other.test(
value) // the order and short-circuit are by-specBut 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-specscalafmt will then reformat to something more correct:
self.test(value) &&
other.test(value) // the order and short-circuit are by-specWith 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.
| @@ -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 | |||
gzm0
Oct 31, 2020
Contributor
These comment moves are unfortunate, not sure we can do anything about them :(
MaximeKjaer
Nov 3, 2020
Author
Contributor
For really long lines, there's nothing to be done, no. This example, however, is fixed by increasing max line length.
| 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)) |
| reversedArgs = | ||
| genPrimitiveJSRepeatedParam(arg) reverse_::: reversedArgs | ||
| reversedArgs = genPrimitiveJSRepeatedParam( | ||
| arg) reverse_::: reversedArgs |
|
|
||
| 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. |
gzm0
Oct 31, 2020
Contributor
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.
kitbellew
Mar 27, 2021
have you tried and of the modes of https://scalameta.org/scalafmt/docs/configuration.html#newlinessource?
|
Last time I seriously looked at scalafmt for Scala.js, I came up with the following .scalafmt.conf: 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. |
Add scalafmt sbt commands. Remove coding style rules deprecated by scalafmt, and scalastyle rules overlapping with scalafmt.
|
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 the 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 of
|
|
Why did the AppVeyor build break? Checking the source files, not much has changed in the offending files: |
That failure is very weird. I've relaunched the build just because I can't make sense of it. |
As discussed in #4037, as of Git 2.23, it is possible to ignore certain commits in a
git blame(see this 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 the
Scala.jsscalafmt preset). However, a few of the current coding style rules cannot be maintained when formatting with scalafmt; an incomplete list is below::in_: Foo | _: Barnewlines.implicitParamListModifierPrefer = beforeat the same time asbinpack.preset = true, so I had to choose between binpacking and preferring line skips before theimplicitkeyword 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:
.git-blame-ignore-revson their Web interface, but there is a community request for it..git-blame-ignore-revsapproach is what is recommended by the (quite popular) Python formatter, black, for migrating coding styles without ruining git blame..git-blame-ignore-revsis locust.git-blame-ignore-revsworks both on the command line, and in VS Code (with no extra configuration; I am using GitLens for viewinggit blamein 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.