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.

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

add scalafmt.conf + apply formatting #546

Merged
merged 2 commits into from Jan 22, 2018

Conversation

Projects
None yet
2 participants
@lorandszakacs
Contributor

lorandszakacs commented Jan 18, 2018

Attempt to solve issue #513

Now, I've added a sample .scalafmt.conf file, and applied it only to Task.scala for the sake of this PR. I glanced over other files, and it's mostly the same kind of formatting.

Leave any specific comments, and I'll try to hammer them out. Unforunately I have no idea how to keep all access modifiers on a separate line from top-level type definitions.

private [pack]
class Test {
//...
}

Always gets collapsed to:

private[pack] class Test {
//...
}

N.B.
You eliminate dangling stuff to the right by moving the closing parenthesis ) on a newline, manually. Otherwise it — sometimes — exhibits the behavior of IntelliJ.

P.S.
sbt scalafmt formats everything. For tight trial and error cycles you can use the IntelliJ plugin.

P.P.S
If you want to check formatting via scalafmt as part of the CI you can simply add scalafmtCheck to the ci aliases in build.sbt

@alexandru

On the private [pack] I don't care except for that space — maybe it's possible to do private[pack].

The availability of that IntelliJ plugin is sweet. Will give it a try.

@@ -285,7 +285,7 @@ import scala.util.{Failure, Success, Try}
*
* @define optionsDesc a set of [[monix.eval.Task.Options Options]]
* that determine the behavior of Task's run-loop.
*
*

This comment has been minimized.

@alexandru

alexandru Jan 18, 2018

Member

Can we disable ScalaDoc formatting?

This comment has been minimized.

@alexandru

alexandru Jan 18, 2018

Member

Well, in this case it isn't so bad, it just removed some whitespace, but I've seen IntelliJ do some weird shit in regards to the indenting of those @define paragraphs.

This comment has been minimized.

@lorandszakacs

lorandszakacs Jan 18, 2018

Contributor

Unfortunately it cannot be disabled completely for docs, you can either choose docstrings=ScalaDoc or docstrings=JavaDoc.

But fortunately, it never messes with the content of the scaladoc, only how the * char is aligned — and in this example I see it removed some whitespace (this is new to me).

Basically, the only real harm it does is this:

  /** Returns a string representation of this task meant for
   * debugging purposes only.
   */

To:

  /** Returns a string representation of this task meant for
    * debugging purposes only.
    */

So no need to worry about it messing around with docs like IntelliJ does. If in the future it ever supports it, then probably it will be configurable (at least disable-able).

this.map { a => f(a); () }
this.map { a =>
f(a); ()
}

This comment has been minimized.

@alexandru

alexandru Jan 18, 2018

Member

Old version is better, sometimes one liners are better, but I can live with it.

if (maxRetries > 0) this.onErrorRestart(maxRetries-1)
else raiseError(ex))
this.onErrorHandleWith(
ex =>

This comment has been minimized.

@alexandru

This comment has been minimized.

@lorandszakacs

lorandszakacs Jan 18, 2018

Contributor

Unfortunately you cannot configure it to always leave the function parameter after the (. And it moves stuff to the next line because the line was too long. But this particular case can be solved by using { instead of (.

This comment has been minimized.

@lorandszakacs

lorandszakacs Jan 18, 2018

Contributor

I've converged a bit more towards the existing style, but this cannot be solved without using the { } lambda syntax.

(implicit cbf: CanBuildFrom[M[Task[A]], A, M[A]]): Task[M[A]] =
def sequence[A, M[X] <: TraversableOnce[X]](
in: M[Task[A]]
)(implicit cbf: CanBuildFrom[M[Task[A]], A, M[A]]): Task[M[A]] =

This comment has been minimized.

@alexandru

alexandru Jan 18, 2018

Member

I don't like this one 😑

Are the styles available documented somewhere?

This comment has been minimized.

@lorandszakacs

lorandszakacs Jan 18, 2018

Contributor

Yeah... unfortunately this is done by it's attempt to break up lines longer than 120 (as configured). You can usually get different results by manually putting certain curried parameter lists on separate lines. But automatically it always gives you these results... I don't think there is a way to configure which parameter lists ought to be moved to a newline, but I'll double check

The only documentation is here:
http://scalameta.org/scalafmt/#Configuration

From there onward it's trial and error. And I try to stay away from the list at the end, where certain properties can be removed without notice in future versions.

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 18, 2018

@alexandru the space in private[pack] was my fault, I just wrote that block of code manually.

I'll fiddle with the config a bit more now.

@codecov

This comment has been minimized.

codecov bot commented Jan 18, 2018

Codecov Report

Merging #546 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   90.43%   90.45%   +0.02%     
==========================================
  Files         366      366              
  Lines        9657     9657              
  Branches     1829     1829              
==========================================
+ Hits         8733     8735       +2     
+ Misses        924      922       -2
@alexandru

This comment has been minimized.

Member

alexandru commented Jan 18, 2018

@lorandszakacs this is a good start. I'll play with that IntelliJ plugin myself.

The problem is carefully reformatting all files ☹️
I guess it's worth it if it provides peace of mind for contributors.

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 18, 2018

@alexandru
If you check the latest commit, you can see what kinds of manual fixes would be necessary to avoid really awkward formatting. I even think I got most of them. I skimmed through the rest of the project diff, and noticed nothing horrible.

62ae4a3

Two important things to note:

  1. run scalafmt twice on a non-formatted codebase, because it might surprise you

  2. In case classes and methods with many parameters always put the ')' on a new line. It's the difference between this:

  def zip6[A1, A2, A3, A4, A5, A6](
    oa1: Observable[A1],
    oa2: Observable[A2],
    oa3: Observable[A3],
    oa4: Observable[A4],
    oa5: Observable[A5],
    oa6: Observable[A6]
  ): Observable[(A1, A2, A3, A4, A5, A6)] =

And this:

  def zip6[A1, A2, A3, A4, A5, A6](oa1: Observable[A1],
                                   oa2: Observable[A2],
                                   oa3: Observable[A3],
                                   oa4: Observable[A4],
                                   oa5: Observable[A5],
                                   oa6: Observable[A6]): Observable[(A1, A2, A3, A4, A5, A6)] =
@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 19, 2018

@alexandru, did you run scalafmt twice on that last commit?

In a minority of cases it is not idempotent for files previously not formatted with scalafmt
scalameta/scalafmt#339

Nevermind:
I checked it locally, and running it again does not change anything anyway. So Everything should be good.

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 19, 2018

@alexandru noticed some awkward right-leaning formatings. Fixing them now.

@lorandszakacs lorandszakacs changed the title from WIP: add scalafmt.conf to add scalafmt.conf + apply formatting Jan 19, 2018

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 19, 2018

@alexandru well, I think I got most awkward code formattings. I removed the "wip" status of this PR.

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 19, 2018

Ok, this time I'm pretty sure I caught everything. Did an IntelliJ search for the regex: \([^,\n)]*,\n. I think this heuristic is enough to find all cases. It yields false positives, but I think no false negatives (of course, I can't be sure).

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 19, 2018

@lorandszakacs I reformatted the whole project and pushed the changes into your branch.

That was a mistake.

Can you please get rid of my commit with a rebase/reset, whatever works? :-)
Also, revert the test formatting that you did, I just want the configuration.

I'm thinking that we should merge the configuration but without reformatting the files at the moment.

Can scalafmt be configured in SBT to do incremental reformatting of changed files? Think I saw in the documentation that it cannot be, but just want to check a second opinion.

Add scalafmt sbt plugin and .scalafmt.conf file
Do not force scalafmt to use dangling parentheses

Add sbt tokens to scalafmt.conf

Add important tip about dangling ) to .scalafmt.conf

@lorandszakacs lorandszakacs force-pushed the lorandszakacs:refactoring/scalafmt branch from 7357345 to d211127 Jan 19, 2018

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 19, 2018

@alexandru kept only the config and the sbt plugin update.

Unfortunately you cannot configure scalafmt to run in such an incremental way. Not even with the alternate neo-sbt-scalafmt plugin.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 19, 2018

@lorandszakacs thanks.

So we can enable automatic reformatting, but once we get some annoyances out of the way.

E.g. I asked on the Gitter channel about a newline rule that annoys me: https://gitter.im/scalameta/scalafmt?at=5a62508a9cdc721e4fa2aa5b

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Jan 19, 2018

@alexandru

Good news, now it should be able to format the entire codebase without introducing abominations like this:

  def zip6[A1, A2, A3, A4, A5, A6](oa1: Observable[A1],
                                   oa2: Observable[A2],
                                   oa3: Observable[A3],
                                   oa4: Observable[A4],
                                   oa5: Observable[A5],
                                   oa6: Observable[A6]): Observable[(A1, A2, A3, A4, A5, A6)] =

Thanks to two configs I had previously missed:

align {
  openParenCallSite = false
  openParenDefnSite = false
}
@alexandru

This comment has been minimized.

Member

alexandru commented Jan 20, 2018

Looks good, now all I'm waiting for is for this option to get released: scalameta/scalafmt#1101 😀

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 22, 2018

It will take some time before a new stable version of scalafmt is released and given that we aren't doing this automatically yet, I'll just merge this configuration for now and activate it to do automatic formatting once that blankLineBeforeDocstring option gets released.

@alexandru alexandru merged commit ba9b58f into monix:master Jan 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lorandszakacs lorandszakacs deleted the lorandszakacs:refactoring/scalafmt branch Apr 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment