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

Update padded-blocks to include padding outside blocks #6445

Closed
wants to merge 1 commit into from

Conversation

ekmartin
Copy link

What/why

This allows ESLint to enforce newline padding above and below blocks, classes and switches.

See #3092 for original discussion.
The issue isn't accepted yet, but I thought I'd open a PR to show one example of how this could be implemented.

Examples

// Turns this:
function a() {

}
function b() {

}

// Into this:
function a() {

}

function b() {

}


// Valid with above: "never" and below: "always":
var a = 5;
var b = function() {

};

var c = 3;


// Valid with above: "never", invalid with above: "always":
var a = "cat";
class B {
  constructor() {}
}
var b = 5; // Valid with below: "never", invalid with below: "always"

How

Currently this extends padded-blocks in a backwards compatible manner by adding the two options above and below. The existing rule options are then moved to inside.

Valid options

// Old (still works):
"always"
"never"
{ "blocks": "always", "switches": "never", "classes": "never" }

// New:
{ "inside": "always" }
{ "inside": { "blocks": "always" }, "above": "always" }
{ "inside": "always", "above": "always", "below": "never" }

I.e. no outside padding is checked without explicitly setting above or below.

Alternatives

Allow toggling based on type (classes, switches and blocks)

Unlike the options for padding on the inside of blocks, above and below only support the options "always" and "never".
This could of course be added, but I think the extra complexity makes less sense here than it does with inside padding.

Merge always/never into outside

This was my original implementation, but I think the extra configurability makes sense here - as there's probably a decent amount of people that would want this to be valid:

var a = 5;
var b = function() {

};

Move this into its own rule

Either by having one rule for all the functionality added here, or by splitting it up into multiple small rules for different block types.

Overlap

This overlaps with #5950, which is a really clean implementation that targets class methods. These are still blocks though, so they're picked up by the functionality implemented in this PR.
This could be "solved" by splitting up this rule into multiple ones, or by ignoring class methods and having both.

Other

Blocks inside switch cases are currently linted as follows:

// Valid with above: "always":
switch (a) {
  case 1: {
  }

  case 2: {
    let b = 5;
    break;
  }
}

Instead of:

switch (a) {
  case 1: {
  }

  case 2: 

  {
    let b = 5;
    break;
  }
}

Also, as the above examples show, outside padding is not checked for the first and last block in another block. This is to prevent overlap with the existing inside padding check.

TODO

  • Update documentation
  • Add more test cases and structure the existing ones a bit better

This enables ESLint to enforce newline padding before and after
blocks, such as if-statements, function declarations and similar.

Error example (should have a newline between the if-statements):
if (a) {
  b();
}
if (b) {
  c();
}
@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @vitorbal, @alberto and @nzakas to be potential reviewers

@nzakas
Copy link
Member

nzakas commented Jun 17, 2016

Thanks for all the great info! It looks like we need to think through the right implementation given everything you've mentioned. We are going to discuss back on the issue to make sure we have our bases covered.

@nzakas nzakas added the do not merge This pull request should not be merged yet label Jun 17, 2016
@notyalca
Copy link

I would like this, and I think it's related to #5982

@kaicataldo
Copy link
Member

We've consolidated this discussion in a new issue: #7356

@kaicataldo
Copy link
Member

kaicataldo commented Feb 7, 2017

@ekmartin Thanks again for contributing to ESLint. The discussion for this is still ongoing (apologies for it taking so long), as we've had difficulty finding a solution for handling conflicts. It's not looking like we'll be using this implementation (we're currently discussing creating an alternate rule), but please feel free to join in the discussion since I know you've thought about this!

Edit: Oops, forgot the issue: #7356

@ekmartin
Copy link
Author

@kaicataldo no worries, I'll close this.

@ekmartin ekmartin closed this Feb 11, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion do not merge This pull request should not be merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants