Skip to content

Require newlines between multiline blocks and expressions#197

Merged
rekmarks merged 1 commit intomainfrom
require-lines-between-blocks
Aug 6, 2021
Merged

Require newlines between multiline blocks and expressions#197
rekmarks merged 1 commit intomainfrom
require-lines-between-blocks

Conversation

@rekmarks
Copy link
Copy Markdown
Member

@rekmarks rekmarks commented Aug 6, 2021

This PR adds another condition where we apply the padding-line-between-statements, namely between multiline blocks and expressions. Whenever we forget to manually add newlines between multiline blocks and expressions, our files become significantly less navigable for those of us that use vim. It also makes our files cramped and harder to read, especially tests.

We already tend to do this in practice, so this is best viewed as enforcing an existing, informal standard.

By way of example, multiline-block-like option results in the following change:

/// before

if (foo) {
  // stuff
}
if (bar) {
  // stuff
}

/// after

if (foo) {
  // stuff
}

if (bar) {
  // stuff
}

While the multiline-expression option results in the following:

/// before

it('does thing one', () => {
  // stuff
})
it('does thing two', () => {
  // stuff
})

/// after

it('does thing one', () => {
  // stuff
})

it('does thing two', () => {
  // stuff
})

@rekmarks rekmarks requested a review from a team as a code owner August 6, 2021 15:55
@rekmarks
Copy link
Copy Markdown
Member Author

rekmarks commented Aug 6, 2021

Extension diff after applying this rule:

 211 files changed, 752 insertions(+)

Some relevant statistics:

  • 90 files (42%) had just 1 insertion
  • 15 files (7%) had 10 or more insertions
    • 13 of those files were tests
    • send.test.js had the most, with 47 insertions
  • There are 1329 .js files in the extension repository, meaning that this change affects ~16% of them

@rekmarks
Copy link
Copy Markdown
Member Author

rekmarks commented Aug 6, 2021

The picture is similar for mobile, with 107 out of 600 .js files (~18%) affected.

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! This seems nice. Breaking change, but that's OK.

@rekmarks rekmarks merged commit 7d8a8ec into main Aug 6, 2021
@rekmarks rekmarks deleted the require-lines-between-blocks branch August 6, 2021 16:45
Gudahtt added a commit that referenced this pull request Dec 12, 2022
This effectively reverts #197. This rule is being removed because we
have found that it forces us to introduce newlines in places where they
obstruct readability rather than helping it.

When grouping statements, it is often useful to "chunk" related lines
together by using a newline before and after the chunk, to denote that
each of the statements are related in some way. This is often done in
unit tests for example, when the "arrange", "act", "assert" grouping
style is used. This rule was forcing us to break up chunks.
Gudahtt added a commit that referenced this pull request Dec 12, 2022
This effectively reverts #197. This rule is being removed because we
have found that it forces us to introduce newlines in places where they
obstruct readability rather than helping it.

When grouping statements, it is often useful to "chunk" related lines
together by using a newline before and after the chunk, to denote that
each of the statements are related in some way. This is often done in
unit tests for example, when the "arrange", "act", "assert" grouping
style is used. This rule was forcing us to break up chunks.
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.

2 participants