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

Align case arrows by default #890

Closed
olafurpg opened this Issue Apr 20, 2017 · 27 comments

Comments

Projects
None yet
9 participants
@olafurpg
Member

olafurpg commented Apr 20, 2017

See sbt/sbt#3125

EDIT, see vote below

@olafurpg olafurpg changed the title from spaces.beforeSeqWildcard = true by default to Update default settings to match with sbt style preference Apr 20, 2017

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Apr 20, 2017

I didn't see this issue before I started writing #891.

I didn't see this issue before I started writing #891.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Apr 20, 2017

What's the current format and what's the newly purposed default, exactly?

sjrd commented Apr 20, 2017

What's the current format and what's the newly purposed default, exactly?

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 21, 2017

Member

@sjrd good question, I've updated the description to include examples.

Member

olafurpg commented Apr 21, 2017

@sjrd good question, I've updated the description to include examples.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Apr 21, 2017

Ah yes, _* should definitely stick together. I've never seen a style where it doesn't.

For the others, I don't care much which one is default. FTR Scala.js uses JavaDoc style and no-space inside import braces, but these two things don't have a widely accepted consensus AFAICT.

sjrd commented Apr 21, 2017

Ah yes, _* should definitely stick together. I've never seen a style where it doesn't.

For the others, I don't care much which one is default. FTR Scala.js uses JavaDoc style and no-space inside import braces, but these two things don't have a widely accepted consensus AFAICT.

@olafurpg olafurpg modified the milestone: v0.7.0 Apr 23, 2017

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Apr 23, 2017

Contributor

Your docstrings example before/after is backwards.

Contributor

dwijnand commented Apr 23, 2017

Your docstrings example before/after is backwards.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Apr 23, 2017

One more customization that sbt/sbt#3125 added was:

# Vertical alignment only => for pattern matching
align.tokens.add = [
  { code = "=>",  owner = "Case"  }
]

single line case alignments in the wild

This alignment is enabled also for scalajs style as AlignToken.caseArrow.

Here's survey of the same projects from #891.

observations

case alignment cannot be said that it's a de facto standard style. But among the three projects that adopted mechanical formatting (Akka, Scala.js and sbt), case alignment rate is 100%. That could be an evidence that it's too annoying to do it for humans, but it might be a good default.

One more customization that sbt/sbt#3125 added was:

# Vertical alignment only => for pattern matching
align.tokens.add = [
  { code = "=>",  owner = "Case"  }
]

single line case alignments in the wild

This alignment is enabled also for scalajs style as AlignToken.caseArrow.

Here's survey of the same projects from #891.

observations

case alignment cannot be said that it's a de facto standard style. But among the three projects that adopted mechanical formatting (Akka, Scala.js and sbt), case alignment rate is 100%. That could be an evidence that it's too annoying to do it for humans, but it might be a good default.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 24, 2017

Member

Thanks @dwijnand, I updated the example.

@eed3si9n Thank you for researching this so thoroughly. Even if I personally dislike vertical alignment, I'm open to include vertical alignment of case arrows in the default style if that's what's most commonly done in the community. I think the benefit of a generally accepted default style outweighs my personal preferences :)

Any suggestion on how we can democratically make this decision? One option is to let people share reactions on a comment. For example, I could comment in a neutral way like this

😄 I don't want vertical alignment for case arrows in the default style
🎉 I want vertical alignment for case arrows in the default style

We can keep the vote open for 1-2 weeks and then the majority wins. WDYT?

PS. This will only affect the default style, people will always be able to disable vertical alignment with align.tokens=[]

Member

olafurpg commented Apr 24, 2017

Thanks @dwijnand, I updated the example.

@eed3si9n Thank you for researching this so thoroughly. Even if I personally dislike vertical alignment, I'm open to include vertical alignment of case arrows in the default style if that's what's most commonly done in the community. I think the benefit of a generally accepted default style outweighs my personal preferences :)

Any suggestion on how we can democratically make this decision? One option is to let people share reactions on a comment. For example, I could comment in a neutral way like this

😄 I don't want vertical alignment for case arrows in the default style
🎉 I want vertical alignment for case arrows in the default style

We can keep the vote open for 1-2 weeks and then the majority wins. WDYT?

PS. This will only affect the default style, people will always be able to disable vertical alignment with align.tokens=[]

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Apr 24, 2017

Contributor

GitHub reactions work - I suggest two comments to vote on rather than trying to repurpose :smile: and :tada:. Another often used alternative is Twitter polls.

Contributor

dwijnand commented Apr 24, 2017

GitHub reactions work - I suggest two comments to vote on rather than trying to repurpose :smile: and :tada:. Another often used alternative is Twitter polls.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 24, 2017

Member

I agree two comments is better. I prefer using Github since many people who have an opinion may not be on Twitter.

Member

olafurpg commented Apr 24, 2017

I agree two comments is better. I prefer using Github since many people who have an opinion may not be on Twitter.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 29, 2017

Member

Sounds good, let's do this then! I propose that we keep this poll open for ~2 week and close on Sunday May 14th (any timezone).

Member

olafurpg commented Apr 29, 2017

Sounds good, let's do this then! I propose that we keep this poll open for ~2 week and close on Sunday May 14th (any timezone).

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 29, 2017

Member

Vote: I don't want vertical alignment for case arrows in the default style

// OK
case a => 1
case aa => 2
Member

olafurpg commented Apr 29, 2017

Vote: I don't want vertical alignment for case arrows in the default style

// OK
case a => 1
case aa => 2
@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 29, 2017

Member

Vote: I want vertical alignment for case arrows in the default style

// OK
case a  => 1
case aa => 2
Member

olafurpg commented Apr 29, 2017

Vote: I want vertical alignment for case arrows in the default style

// OK
case a  => 1
case aa => 2
@ShaneDelmore

This comment has been minimized.

Show comment
Hide comment
@ShaneDelmore

ShaneDelmore Apr 29, 2017

Contributor

My experience backs @eed3si9n thoughts here, I have traditionally preferred no alignment even though I have seen compelling examples where it improved readability for two reasons. 1. I don't want to have to change other lines when I add a new longer case. 2. It clutters diffs.

These days every diff tool I can think of including github can ignore whitespace in diffs and when automated there is no maintenance cost to other devs to update alignment so I am not seeing people complain about it as much when it's automated.

Contributor

ShaneDelmore commented Apr 29, 2017

My experience backs @eed3si9n thoughts here, I have traditionally preferred no alignment even though I have seen compelling examples where it improved readability for two reasons. 1. I don't want to have to change other lines when I add a new longer case. 2. It clutters diffs.

These days every diff tool I can think of including github can ignore whitespace in diffs and when automated there is no maintenance cost to other devs to update alignment so I am not seeing people complain about it as much when it's automated.

@olafurpg olafurpg modified the milestones: v0.7.0, v1.0 Apr 29, 2017

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Apr 29, 2017

To make the options stand out could you put YES and NO like:

Vote: NO, I don't want vertical alignment for case arrows in the default style

Vote: YES, I want vertical alignment for case arrows in the default style

To make the options stand out could you put YES and NO like:

Vote: NO, I don't want vertical alignment for case arrows in the default style

Vote: YES, I want vertical alignment for case arrows in the default style

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Apr 30, 2017

Member

NO

Member

xeno-by commented Apr 30, 2017

NO

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 30, 2017

Member

@eed3si9n I added the examples to avoid confusion, I thought assigning NO and YES to either option might cause bias.

Member

olafurpg commented Apr 30, 2017

@eed3si9n I added the examples to avoid confusion, I thought assigning NO and YES to either option might cause bias.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Apr 30, 2017

Member

@xeno-by Please upvote with 👍 to your prefered option. Commenting "NO" does not count.

Member

olafurpg commented Apr 30, 2017

@xeno-by Please upvote with 👍 to your prefered option. Commenting "NO" does not count.

@ShaneDelmore

This comment has been minimized.

Show comment
Hide comment
@ShaneDelmore

ShaneDelmore May 1, 2017

Contributor

The example confused me enough I voted for the wrong one. Oops. Fixed now. The confusion for me was that there was a positive and negative example in each voting option. If we do more of these in the future it may be more clear to just put the positive example in each comment.

Contributor

ShaneDelmore commented May 1, 2017

The example confused me enough I voted for the wrong one. Oops. Fixed now. The confusion for me was that there was a positive and negative example in each voting option. If we do more of these in the future it may be more clear to just put the positive example in each comment.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg May 1, 2017

Member

@ShaneDelmore Good point, I've updated the comments to only include the OK case.

Member

olafurpg commented May 1, 2017

@ShaneDelmore Good point, I've updated the comments to only include the OK case.

@pjrt

This comment has been minimized.

Show comment
Hide comment
@pjrt

pjrt May 1, 2017

Collaborator

Though I prefer alignment, this isn't something I feel should be in the default. The reason being:

  1. I've actually never seen it in the wild for Scala (only on my own stuff).
  2. Given the history we have with alignment in Scalafmt (kind of buggy), we should be hesitant to put it in default.
Collaborator

pjrt commented May 1, 2017

Though I prefer alignment, this isn't something I feel should be in the default. The reason being:

  1. I've actually never seen it in the wild for Scala (only on my own stuff).
  2. Given the history we have with alignment in Scalafmt (kind of buggy), we should be hesitant to put it in default.
@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican May 1, 2017

These are my reasons to vote against this proposal:

  • When adding a new case that is longer than the rest, the diff will report on the whole match case being changed when only a new clause did.
  • Column char limit does not affect case alignment, e.g. you can have a case statement being 250 chars long even when the maxColumn option is 100.

The only reason I see others may have to vote in favor of it is "better readability". However, I don't consider it to be more readable than its counterpart. Long cases force you to scroll (or look) horizontally rather than vertically and that disturbs the natural way of reading text.

In any case, I'm happy with the result of the poll.

jvican commented May 1, 2017

These are my reasons to vote against this proposal:

  • When adding a new case that is longer than the rest, the diff will report on the whole match case being changed when only a new clause did.
  • Column char limit does not affect case alignment, e.g. you can have a case statement being 250 chars long even when the maxColumn option is 100.

The only reason I see others may have to vote in favor of it is "better readability". However, I don't consider it to be more readable than its counterpart. Long cases force you to scroll (or look) horizontally rather than vertically and that disturbs the natural way of reading text.

In any case, I'm happy with the result of the poll.

@pjrt

This comment has been minimized.

Show comment
Hide comment
@pjrt

pjrt May 1, 2017

Collaborator

In any case, I'm happy with the result of the poll.

I'm actually not happy with the results (currently align is winning by a large margin). I feel people will pick the option that looks better over the one that is easier to implement and maintain. That one, ofc, being the aligned one.

Collaborator

pjrt commented May 1, 2017

In any case, I'm happy with the result of the poll.

I'm actually not happy with the results (currently align is winning by a large margin). I feel people will pick the option that looks better over the one that is easier to implement and maintain. That one, ofc, being the aligned one.

@ShaneDelmore

This comment has been minimized.

Show comment
Hide comment
@ShaneDelmore

ShaneDelmore May 2, 2017

Contributor

@jvican This probably won't change your mind but if it helps you you can add ?w=1 to the end of different in github to hide whitespace. There are also browser plugins to hide it by default.

Contributor

ShaneDelmore commented May 2, 2017

@jvican This probably won't change your mind but if it helps you you can add ?w=1 to the end of different in github to hide whitespace. There are also browser plugins to hide it by default.

@hseeberger

This comment has been minimized.

Show comment
Hide comment
@hseeberger

hseeberger May 7, 2017

Vote: I want vertical alignment for case arrows in the default style

Vote: I want vertical alignment for case arrows in the default style

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand May 7, 2017

Contributor

I think case a/case aa doesn't properly demonstrate aligned vs not aligned. Here's an example from the sbt/sbt codebase. Which one of these would you like to see in your codebase and in other codebases using scalafmt's default options?

option match {
  case JarManifest(mergeManifest)          => mergeManifests(manifest, mergeManifest)
  case MainClass(mainClassName)            => main.put(Attributes.Name.MAIN_CLASS, mainClassName)
  case ManifestAttributes(attributes @ _*) => main.asScala ++= attributes
  case _                                   => log.warn("Ignored unknown package option " + option)
}

or

option match {
  case JarManifest(mergeManifest) => mergeManifests(manifest, mergeManifest)
  case MainClass(mainClassName) => main.put(Attributes.Name.MAIN_CLASS, mainClassName)
  case ManifestAttributes(attributes @ _*) => main.asScala ++= attributes
  case _ => log.warn("Ignored unknown package option " + option)
}
Contributor

dwijnand commented May 7, 2017

I think case a/case aa doesn't properly demonstrate aligned vs not aligned. Here's an example from the sbt/sbt codebase. Which one of these would you like to see in your codebase and in other codebases using scalafmt's default options?

option match {
  case JarManifest(mergeManifest)          => mergeManifests(manifest, mergeManifest)
  case MainClass(mainClassName)            => main.put(Attributes.Name.MAIN_CLASS, mainClassName)
  case ManifestAttributes(attributes @ _*) => main.asScala ++= attributes
  case _                                   => log.warn("Ignored unknown package option " + option)
}

or

option match {
  case JarManifest(mergeManifest) => mergeManifests(manifest, mergeManifest)
  case MainClass(mainClassName) => main.put(Attributes.Name.MAIN_CLASS, mainClassName)
  case ManifestAttributes(attributes @ _*) => main.asScala ++= attributes
  case _ => log.warn("Ignored unknown package option " + option)
}
@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg May 24, 2017

Member

Thank you everyone who participated in the voting! That seems to be a strong victory for vertical alignment in case arrows, for better or worse 😄 Those who dislike vertical alignment will always be able to disable this change in the default style with align.tokens = [].

I have set the deadline for the v1.0 milestone on June 5th https://github.com/scalameta/scalafmt/milestones/12

Member

olafurpg commented May 24, 2017

Thank you everyone who participated in the voting! That seems to be a strong victory for vertical alignment in case arrows, for better or worse 😄 Those who dislike vertical alignment will always be able to disable this change in the default style with align.tokens = [].

I have set the deadline for the v1.0 milestone on June 5th https://github.com/scalameta/scalafmt/milestones/12

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Jun 8, 2017

Member

I just merged #964, which changes the default style to align on case arrows Along the way, style=defaultWithAlign is no longer documented as a provided style. Instead, the align settings now supports four alternatives

align = none # absolutely nothing
align = some # default, open parens and case arrow
align = more # style=defaultWithAlign
align = most # new experimental setting that enables more than "more" :)

Along the way, the docs for the align setting and it's nested keys have been greatly expanded.

Thank you everyone who participated in the discussions and voted!

Member

olafurpg commented Jun 8, 2017

I just merged #964, which changes the default style to align on case arrows Along the way, style=defaultWithAlign is no longer documented as a provided style. Instead, the align settings now supports four alternatives

align = none # absolutely nothing
align = some # default, open parens and case arrow
align = more # style=defaultWithAlign
align = most # new experimental setting that enables more than "more" :)

Along the way, the docs for the align setting and it's nested keys have been greatly expanded.

Thank you everyone who participated in the discussions and voted!

@olafurpg olafurpg closed this Jun 8, 2017

@olafurpg olafurpg changed the title from Update default settings to match with sbt style preference to Align case arrows by default Jun 8, 2017

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