Skip to content
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

Avoid empty lines in the beginning and end of blocks #1431

Merged

Conversation

tdidriksen
Copy link
Contributor

@tdidriksen tdidriksen commented Jun 7, 2019

Previously, Scalafmt would preserve blank lines at the beginning and end of blocks like this

def other: String = {

  val aux = {

    "Result"

  }

  aux

}

Now, Scalafmt removes those blank lines producing this output instead:

def other: String = {
  val aux = {
    "Result"
  }

  aux
}

This behavior is enabled by default. Users can keep the old behavior with the new edition configuration

edition = 2019-10

Added
newlines.avoidEmptyLineAfterOpenCurly = true
newlines.avoidEmptyLineBeforeClosingCurly = true

The intention is to remove empty lines appearing
at the start of block (i.e. immediately after the opening
curly brace), and the end of a block (i.e. immediately
before the closing curly brace.
avoidAfterYield: Boolean = true
avoidAfterYield: Boolean = true,
avoidEmptyLineAfterOpenCurly: Boolean = false,
avoidEmptyLineBeforeClosingCurly: Boolean = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great contribution, I want it in my .scalafmt.conf 😄

Is there a use case when this settings will be turned on separatly? May be replace it by one configuration key (something like avoidEmptyLinesAroundBlock)?

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 split it into two settings to make it as flexible as possible, but I'd also always want to have both settings turned on. One configuration key would be fine with me.

Copy link
Member

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much your contribution @tdidriksen
I'm sorry that I'm very reluctant to add any new configurations to scalafmt...
Personally removing empty lines before & after the curly braces looks nice, but scalafmt inherently do preserve a blank line and it conflicts with scalafmt's default behavior (I know that that's why you are trying to add this feature as a configurable option).

@tdidriksen
Copy link
Contributor Author

@tanishiking
I see, though I believe the presence/absence of blank lines is an important part of code formatting as well. Does this mean that there is no way this PR is going to be accepted, or can I make some modifications that would make it acceptable?

@tanishiking
Copy link
Member

@tdidriksen I'm sorry for the long absence from this PR 🙇
As @poslegm said, it would be nicer to have avoidEmptyLinesAroundBlock (or something), instead of having two different options.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the goal to not introduce new off-by-default options. I'm personally fine with making this the default behavior since I have never liked the optional empty lines around blocks.

As a policy however, we should avoid changing the default formatting output between releases without a fallback to the old formatting style. The motivation for this policy is that users should never feel the need to stay on an older release to keep some formatting style. Having everybody on the latest Scalafmt version is desirable for various reasons.

I propose we introduce something like compat setting below inspired by Rust editions https://doc.rust-lang.org/edition-guide/editions/index.html

edition = 2019-10

The setting works as follows: edition = 2019-10 means that you are using the default settings from October 2019. Whenever we change the default formatting behavior, we introduce a new valid legacy default value of that year/month, for example edition = 2020-02. The end goal for users is to remove this setting from their .scalafmt.conf when they have time to work on the upgrade (which often requires synchronization with your team).

An important aspect of this is that users will not be able to pick and choose individual changes to the default formatting behavior. Users must stay on one edition and there is incentive to stay on the latest edition in order to enjoy the latest defaults.

The edition setting can be implemented with metaconfig something like this

sealed abstract class Edition(val order: Int, val name: String)
object Edition  {
  implicit val ordering = Ordering.by[Edition, Int](_.order)
  implicit val decoder = metconfig.ConfDecoder....
  implicit val encoder = metconfig.ConfEncoder....
}
case object Edition201910 extends Edition(0, "2019-10")
case object EditionLatest extends Edition(Int.MaxValue, "latest")

def isEmptyLineBetweenBlocks = config.edition > Edition201910

Conflicts:
	scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala
Adding one option instead, newlines.avoidEmptyLinesAroundBlock
This option is on by default.
Since newlines.avoidEmptyLinesAroundBlock on by default,
existing tests should take it into account
If no edition has been configured, default to the latest edition
@tdidriksen
Copy link
Contributor Author

tdidriksen commented Oct 25, 2019

@olafurpg @tanishiking
I've merged the two options into one, newlines.avoidEmptyLinesAroundBlock.
The behaviour is unchanged, but it is now on for the latest edition.

I have also taken a shot at introducing the suggested idea of editions, which seems to work out quite well. I have only used the edition for this one config option for now. Let me know what you think.

For now, it is still possible to override the value of newlines.avoidEmptyLinesAroundBlock in your config.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing the edition idea and updating the all the test cases 🙏 Two minor comments, otherwise looks good to me

@@ -432,6 +432,39 @@ else {
}
```

### `newlines.avoidEmptyLinesAroundBlock`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section can be removed now.

) {
val reader: ConfDecoder[Newlines] = generic.deriveDecoder(this).noTypos

def shouldAvoidEmptyLinesAroundBlock(edition: Edition): Boolean = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this to a val inside ScalafmtConfig

@olafurpg
Copy link
Member

@poslegm @tanishiking what do you think about introducing editions?

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 I would like to hear from @poslegm and @tanishiking before going ahead with the merge.

@olafurpg olafurpg changed the title Configuration for avoiding empty lines in the beginning or end of blocks Avoid empty lines in the beginning and end of blocks Oct 25, 2019
@olafurpg
Copy link
Member

@tdidriksen I took the liberty to update the PR description and title to reflect the latest changes

@poslegm
Copy link
Collaborator

poslegm commented Oct 25, 2019

I fully agree with idea of reasonable and actual defaults. Editions will be very useful but a little bit complicated mechanic for users (version and edition — two versioning variables in config)

I am guessing that most users won't know about it :) So we need to document editions and versioning mechanism well at the top of configuration doc. I think that editions documentation is not necessary in this PR, but we should create issue so we don't forget it.

@poslegm poslegm closed this Oct 25, 2019
@tanishiking
Copy link
Member

tanishiking commented Oct 25, 2019

@olafurpg
I like that idea, and I would give it the thumbs up!
That policy makes scalafmt easy to move on to the new default behavior, giving scalafmt users a time to adjust their team to the new behavior 😍

I have one concern about the format of the edition value (like 2019-10), and I propose scalafmt to use edition = <scalafmt-core's version> (like edition = 2.2.1) instead.
Since Rust ships releases on a six-week cycle, it is reasonable to label the edition values with year-month format.
However, on the other hand, scalafmt doesn't have a monthly release cycle, but it ships the new versions occasionally. This mismatch between the release cycle and edition format might cause

  • Users may be confused about which edition corresponds to which version of scalafmt.
  • If we ship two releases in a month, with changes in default behavior (I hope it won't happen though), we might need to define a value with an irregular label (like "2019-10-2" because there's already "2019-10").

And, except my concern about the edition values, this change looks LGTM 👍 @tdidriksen

@poslegm poslegm reopened this Oct 25, 2019
@neko-kai
Copy link

@olafurpg

An important aspect of this is that users will not be able to pick and choose individual changes to the default formatting behavior. Users must stay on one edition and there is incentive to stay on the latest edition in order to enjoy the latest defaults.

This seems far too opinionated to me. I like the idea of versioning defaults, but not being able to mix the settings of different editions in the case when none of the editions suit you sounds really user-unfriendly – note that we can't really just borrow exactly what highly opinionated languages like Go & Rust do, since Scala's not like that historically.

@olafurpg
Copy link
Member

My motivation for this disallowing users to pick and choose individual settings going forward

  • make the project more maintainable: it's really difficult to reason how all possible combinations of off-by-default settings work in practice, let alone test it.
  • improve the user experience: currently, users can mix and match individual settings that hit on bugs such as verticalMultiline disables optIn.configStyleArguments #1539.

@poslegm
Copy link
Collaborator

poslegm commented Oct 25, 2019

@olafurpg I'm not sure that date in config would motivate users to update better than old version. But version allow us to release two editions per month as @tanishiking says.

What about Ordering instance for editions? Users will be able to just write random version or month and don't think is there case object in our sources.

Also I propose more detailed name for this setting: default-config-edition. edition word may be confusing (edition of what?).

@olafurpg
Copy link
Member

What about Ordering instance for editions? Users will be able to just write random version or month and don't think is there case object in our sources.

I think it's a good idea to could allow all year/month combinations, so that users can pin the edition to the month they upgrade Scalafmt even if that month had no Scalafmt release. I would prefer to require a consistent format, allowing users to choose either a version of date would be confusing IMO.

But version allow us to release two editions per month

It's rare that we have multiple stable releases in the same month that change the formatting behavior. If that happens I would be inclined to limit such releases to once per month even if that comes at the price of having to wait until the next month begin to release a new change. We could go the route of allowing edition = 2019-10-25 if we want to support more precision but I don't think it should be necessary.

Also I propose more detailed name for this setting: default-config-edition. edition word may be confusing (edition of what?).

It would be the edition of Scalafmt. I don't expect we'll have more than one "edition" setting. Would it maybe be clearer if the setting was named "date" or "lastUpdated" instead?

@dwijnand
Copy link
Contributor

My motivation for this disallowing users to pick and choose individual settings going forward

  • make the project more maintainable: it's really difficult to reason how all possible combinations of off-by-default settings work in practice, let alone test it.
  • improve the user experience: currently, users can mix and match individual settings that hit on bugs such as verticalMultiline disables optIn.configStyleArguments #1539.

I can absolutely sympathise with that, and being more conservative on how many flags you expose. But perhaps throwing them all out and banning any more is a little overboard.

@poslegm
Copy link
Collaborator

poslegm commented Oct 25, 2019

It would be the edition of Scalafmt

What functionality besides default configuration do you want to control with edition?

I think that “scalafmt edition” is better googlable than “scalafmt date” or “scalafmt lastUpdated”.

In summary:

  • I am OK with editions by months and breaking releases limit. But prefer editions by versions variant.
  • We need Ordering instance for editions. UPD: oops, Ordering is already here 😄
  • We need clear documentation for editions
  • Users should be able to use scalafmt without knowledge about editions

In my opinion this PR can be merged as is but released only after editions is fully completed.

@poslegm
Copy link
Collaborator

poslegm commented Oct 25, 2019

An important aspect of this is that users will not be able to pick and choose individual changes to the default formatting behavior. Users must stay on one edition and there is incentive to stay on the latest edition in order to enjoy the latest defaults.

Step-by-step change of parameters between editions can be helpful for large codebases update. Let users rule their configs themselves. Motivation for new editions update is good point but not at that price 🙏

@olafurpg
Copy link
Member

olafurpg commented Oct 26, 2019

But perhaps throwing them all out and banning any more is a little overboard.

This is not a proposal to throw out all settings, existing configuration option will continue to work. The goal for editions is exactly to become more conservative on how many new flags we expose.

@olafurpg
Copy link
Member

Step-by-step change of parameters between editions can be helpful for large codebases update.

The motivation for editions is exactly to give large codebases flexibility to choose when to upgrade to the latest Scalafmt formatting output.

Let users rule their configs themselves.

Users will be able to continue choosing from existing configuration options, no settings are going away. Going forward, editions impose the restriction that new changes to the formatting output need to be well-enough received that they are enabled by default.

Maintaining the status-quo is not sustainable. As a maintainer it's impossible to reason about every possible combination of options that users come up with. As a user the tool is difficult to understand because it's not clear what settings are safe to use with each other.

@olafurpg
Copy link
Member

note that we can't really just borrow exactly what highly opinionated languages like Go & Rust do, since Scala's not like that historically.

I don't think there's a fundamental or historical reason why Scala can't have opinionated tooling, see

The Python and JavaScript communities are several orders of magnitude larger than the Scala community.

@neko-kai
Copy link

@olafurpg
Python and JS are also large enough to have multiple formatting tools, so it's not a big deal for them to include a couple zero configuration ones.

I wouldn't be so uncomfortable with the idea if scalafmt's defaults weren't also it's own unique opinions – e.g. scalafmt's default of 4 spaces in constructors is in conflict with example formatting in the Scala Style Guide – https://docs.scala-lang.org/style/declarations.html – there are more examples, but the point is not at all to criticize, but to illustrate that if anything, as a user I'd like to have more options to opt out of scalafmt's defaults – not less!

@tanishiking
Copy link
Member

@olafurpg

This approach could work as well. My motivation to use the year-month format was to encourage people to upgrade to the latest edition.

It makes sense. Indeed it is also a bit confusing to have edition = 2.1.2 or something. Now I'm supporting year-month format if it is well-documented.

If that happens I would be inclined to limit such releases to once per month even if that comes at the price of having to wait until the next month begin to release a new change.

👍 I was just too worried about that.


@poslegm

Users should be able to use scalafmt without knowledge about editions

Yeah, it is an important point, and
Most users will be able to continue to use scalafmt without knowledge about edition I think. The only users, who find that it takes time to upgrade the scalafmt to the latest, have to concern about it.
On top of that, the behavior of edition is easy to understand, and I don't think this makes scalafmt so complicated.

Also I propose more detailed name for this setting: default-config-edition. edition word may be confusing (edition of what?).

I agree with @olafurpg. edition means the edition of Scalafmt itself, and the edition is the most straightforward and clear option.


@dwijnand

perhaps throwing them all out and banning any more is a little overboard.

We didn't mean to ban any more options. If we think that the new option is worth maintaining, we can add new options of course :)
The intention of edition is to make users more comfortable with upgrading scalafmt, and make it easier to change default behavior instead of exposing more and more options because more options exposed, it would be more difficult to grasp the behavior of scalafmt under the combination of some settings.

* support any year-month combination instead of only 2019-10
* add documentation explaining how `edition` works
@olafurpg
Copy link
Member

@tdidriksen I opened famly#1 to address a few comments in this PR

…d_of_blocks

Address feedback on editions
@tdidriksen
Copy link
Contributor Author

@olafurpg Merged your changes.

@poslegm
Copy link
Collaborator

poslegm commented Oct 28, 2019

@tdidriksen @olafurpg Thank you very much for your participation! newlines.avoidEmptyLinesAroundBlock and edition will be very helpful for scalafmt users.

@poslegm poslegm merged commit 609dfdb into scalameta:master Oct 28, 2019
olafurpg pushed a commit to olafurpg/scalafmt that referenced this pull request Oct 28, 2019
Followup from scalameta#1431. This
setting is already covered by the `edition` option.
@olafurpg
Copy link
Member

I just realized this PR doesn't fully implement the edition proposal since it's still possible to configure newlines.avoidEmptyLinesAroundBlock = true. I opened #1541 to remove this option. Keeping around hidden/undocumented settings for people who dig into the source code is not a sustainable way to maintain this project.

@gabro
Copy link
Member

gabro commented Oct 28, 2019

Python and JS are also large enough to have multiple formatting tools, so it's not a big deal for them to include a couple zero configuration ones.

FWIW, I know the JS ecosystem a bit and I don't think this is true for JS.

Prettier is the only mainstream formatting tool (proper) I know of and you can't configure much other than the line length and other minor details (semicolons and string quotes are the ones I know of).

Some people may also use ESLint with automatic fixes for "formatting rules", but I wouldn't consider it a formatting tool since it performs token-based fixes and it's not able to rewrite ASTs in full.

olafurpg pushed a commit to olafurpg/scalafmt that referenced this pull request Oct 28, 2019
Followup from scalameta#1431. This
setting is already covered by the `edition` option.
@ekrich
Copy link

ekrich commented Oct 28, 2019

I wish we could have one mostly agreed upon standard Scala format. I think that you get used to what you see and everyone finding their own perfect format is a huge waste of time and it makes it harder to read code bases with different styles.

@eatkins
Copy link

eatkins commented Nov 8, 2019

Just chiming in that I also am in favor of removing configuration of formatting options. In addition to the javascript and python examples @olafurpg provided, neither the google java formatter (which I believe is the only widely used java code formatter) nor gofmt are configurable. Using the google java formatter convinced me that not having options is better. I don't love all of the rules, but it's much better to just submit to those conventions and worry about more important things.

I can't speak to the maintenance cost of supporting configurable options but I'd naively assume it's higher than not having them. From the perspective of the global scala ecosystem, I think that providing options has a net negative overall effect both because it reduces consistency of formatting across projects and it encourages users to spend time tweaking options which then will in turn cause other users to spend time trying to figure out why the formatting isn't as expected. In my opinion, there are better ways for people to spend their time.

Finally 👍 for editions because changing the options can cause huge diffs and it would be nice to manage when we switch over to the new formatting rules.

@dwijnand
Copy link
Contributor

dwijnand commented Nov 8, 2019

I just want to point out that this isn't an either/or situation (this one in particular is a good example): scalafmt can also choose not to get into the business of trimming (or adding) empty lines in the beginning or the end of blocks.

I think it's why I took to liking scalafmt over scalariform: it made select, known-to-be-good changes, and left other things as they were.

Also see #1546 (comment) where I explain how editions aren't implemented how one might've assumed.

@eatkins
Copy link

eatkins commented Nov 8, 2019

Fair enough. I don't care that much about this specific formatting option, I just noticed that much of the discussion was around configuration options in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants