scalafmtTest doesn't complain about trailing whitespace

[densh]

For example this file, has plenty of trailing whitespace that is not detected:

https://github.com/scala-native/scala-native/blob/master/nativelib/src/main/scala/scala/scalanative/runtime/Arrays.scala

Here is the same file with trailing whitespace highlighted with red:

https://www.dropbox.com/s/grnn1qs6ic94xfe/Screenshot%202016-06-27%2015.54.31.png?dl=0

Using:

• 0.2.5
• --style defaultWithAlign --javaDocs

[densh]

Needless to say running scalafmt doesn't fix this, the trailing whitespace remains to be there after formatting.

[olafurpg]

scala.meta can't parse that file so scalafmt leaves it unformatted

ollie-target@ new java.io.File("Arrays.scala").parse[Source]
res2: Parsed[Source] = Arrays.scala:190: error: ; expected but = found
!(arr.cast[Ptr[Byte]] + sizeof[Ptr[_]]).cast[Ptr[Int]] = length
                                                           ^

I'll open a ticket in scala.meta.

[densh]

Silently ignoring files that don't parse might not be the best stategy for scalafmtTest. It should complain if anything at all is wrong, anywhere.

[olafurpg]

It should complain if anything at all is wrong, anywhere.

I like the incremental development approach taken in rustfmt. Until scalafmt becomes perfect, instead of stopping the world I prefer to leave files that trigger errors alone. For example, in 0.2.6, I removed best-effort formatting when the search state explodes in favour of leaving the file unformatted. To complement this fail-silent approach, I added a --debug flag to the CLI to see errors that scalafmt encounters.

[densh]

But than it's not useful as CI validation tool, which kills the main benefit of not having to ever discuss formatting on PRs. Having it as part of CI means that you trust formatting tool to let you know if something is wrong and being able to fix it.

I'm fine with having it under a flag or something (e.g. --strict ?).

[densh]

The benefit of CI integration is big enough that I'm personally fine with the current quirks of the formatter. But I'm only willing to tolerate those because of the code uniformity gains it provides. If there are no guarantees, it's essentially worst of both worlds: you leave with the quirks AND you must somehow check everything manually at the same time.

[xeno-by]

I can publish 1.1.0-SNAPSHOT with a fix. Would that be enough for you guys to unblock formatting of scala-native? upd. Published.

[xeno-by]

Fixed in https://github.com/scalameta/scalameta/pull/444. @olafurpg you may be interested in revisiting your assumptions about Term.Assign and Term.ApplyUnary.

[olafurpg]

@xeno-by There is no rush. I still haven't merged the scala.meta 1.0 migration so I will need to do that first.

@densh It seems that scalafmt fails on 5 files in the scala-native repo, 3 files trigger that parse error that's now been fixed and 2 files cause a search state explosion.

./nativelib/src/main/scala/scala/scalanative/runtime/package.scala: scala.meta.parsers.ParseException: ; expected but = found at 933..934
./nativelib/src/main/scala/scala/scalanative/runtime/Arrays.scala: scala.meta.parsers.ParseException: ; expected but = found at 8748..8749
./demo/jvm/smallpt.scala: org.scalafmt.Error$SearchStateExploded: Search state exploded
./demo/native/smallpt.scala: scala.meta.parsers.ParseException: ; expected but = found at 2657..2658
./nscplugin/src/main/scala/scala/scalanative/nscplugin/NirCodeGen.scala: org.scalafmt.Error$SearchStateExploded: Search state exploded

It would be possible to add a --strict option to the scalafmtTest so that you at least know when scalafmt fails. However, I think that flag won't be useful until scalafmtTest is smart enough to only look at the diff in a PR. Currently, scalafmtTest checks the whole repo so the --strict flag will cause it to fail on every PR.

Unfortunately, if this is a blocker for you then you might want to consider disabling scalafmt in you CI build. I won't have time to attend these issues until around August.

[densh]

Doesn't state space explosion correlate closely with deeply nested code that should be refactored anyway? IMO both of the errors should be reported in some form. Having those as warnings would be a good start before we're reasonably sure that scala.meta can parse everything scalac can.

[olafurpg]

Doesn't state space explosion correlate closely with deeply nested code that should be refactored anyway?

True, several people have suggested that scalafmt should issue linter warnings so I opened #290 a while ago. Deeply nested code is a good example of warnings that scalafmt could complain about.

[olafurpg]

0.4.1 depends on scala.meta 1.1.0 which should be able to format that file now. Can you confirm @densh?

Since 0.3.x, the SBT plugin now prints out a warning on failure to format a file. Unfortunately, it's not yet possible to configure the warnings to be errors.

[densh]

@olafurpg It's able to format the file now, I confirm that. 👍