eslint 🚀 - add outer padding to rule padded-blocks and cover existing jscs functionality

Closing the other issues so we can consolidate the discussion in one issue. Thanks @quinn!

@eslint/eslint-team Can we get some opinions from team members? @TheSavior and @quinn have done a great job of pushing forward with this on their own but I think we need the team involved.

kaicataldo on 13 Oct 2016

@kaicataldo Anything I can do to help? I would be happy to try an implementation but there are already multiple PRs and I don't want to redo anyone else's work. I could try to merge the existing PRs or something. If there's anything I can do let me know. thanks :)

@kaicataldo Maybe the discussion from this issue should be moved here? https://github.com/eslint/eslint/issues/7145 In my example above, I only included the "singleLine" exception for "outer" padding, not inner, since it was not part of the current "padded-blocks" / jscs implementation. "singleLine" I think it a little bit odd in that list because it's slightly different than the other options (all of the other options have to do with the location of the block rather than the structure of the block). idk hopefully i am not making things more complicated :p

quinn on 17 Oct 2016

👍1

@quinn Sorry for not responding sooner - I think it's fine to leave that as a separate discussion so we can focus on padding outside blocks. Hopefully we'll get some discussion on here soon - thanks for your patience.

kaicataldo on 4 Nov 2016

Thanks for the proposal. This looks good, but I have a few questions/comments:

What are the proposed defaults for these options?
I think the outerExceptions and innerExceptions should have better names; inCallExpressions, switches, etc. seem inconsistent to me. Maybe it would be better if it was just a node type: outerExceptions: ["CallExpression", "SwitchStatement"]
Would it be possible to have two separate objects for inner and outer? For example:

json {"inner": {"padding": "always", "exceptions": ["NewExpression"]}, "outer": {"padding": "never"}}

not-an-aardvark on 4 Nov 2016

@not-an-aardvark

i * think * the defaults would simply be { "inner": "always" } since that would match (i think) what happens when you use padded-blocks without options currently.
the outerExceptions were just pulled from the original jscs rule, but you're right they aren't consistent with the rest of it. as far as i've seen it seems like eslint tends to go with short option names without camel case.
yeah, that seems cleaner to me. maybe if you aren't specifying any exceptions you can still set the padding type directly: { "inner": "never", "outer": "always" }. Are there any existing eslint rules that have nested options like that?

quinn on 4 Nov 2016

Maybe it's just me, but this seems like too much to combine into padded-blocks. Is there a way we can keep padded-blocks focused on the inner extra lines and create a new rule to deal without outer extra lines? Or is there some relationship here that I'm missing?

nzakas on 4 Nov 2016

@nzakas The issue is that the two rules would conflict with each other:

/* eslint padded-blocks: [error, "always"] */
/* eslint padding-outside-blocks: [error, "never"] */

{
  function foo() {
  } // should there be an empty line below? padded-blocks says yes, the other rule says no

}

not-an-aardvark on 4 Nov 2016

👍1

@nzakas @not-an-aardvark yeah. that conflict is one of the causes of the lengthy discussion. keeping it as a separate rule was discussed, but I think someone indicated it would be more weird to have a rule behave differently based on the.. rules of another rule. thats the reason for the prefer option above. I think it could still be simple to use, there are just these extra configurations available..

quinn on 4 Nov 2016

What about changing concept totally and split the rule in:

padding inside blocks
padding between blocks (of the same scope/indent)

In this way there would be no conflict, as far as I can see.
And the rule would be simplified.

caesarsol on 5 Nov 2016

To further elaborate, reading the JSCS rule in question makes it clear thet it's a quite different subject, only regarding padding after blocks.

My proposal does not cover the block-statement boundaries but only the block-block boundaries.
Could be an entirely new rule, or they could be joined.

I think a new small focused rule is better than a large number of parameters for an already existing rule.

What do you think?

caesarsol on 5 Nov 2016

@caesarsol I think the only reason all of this is being considered as a single rule is because of the situations where the 2 rules could conflict and there isn't a precedent in eslint already for what to do when 2 rules say to a different thing for a given statement (afaik) does that help or did I miss the point of what you were saying ??

quinn on 5 Nov 2016

I'm proposing about avoiding the conflict! Maybe with a new concept of block boundaries.
(You may have missed my first post just before)

caesarsol on 5 Nov 2016

@caesorsol ok, the issue isn't between blocks, it's when a block has inside of it a block as the first or last thing.. does that make sense? Also, I personally want padding around my blocks even if the things on either side are not blocks (except in the case of a wrapping block..)

quinn on 5 Nov 2016

(Sorry, it was difficult for me to explain)
Wouldn't the conflict issue be resolved if we reason about:

boundary between two blocks
boundary between a block and a piece of code

For each of those a rule about enforcing the newline.

This would certainly leave the inner block padding alone, making space for the rule without conflicts.

Any downside you can think of?

caesarsol on 5 Nov 2016

Many of our rules that enforce blank newlines before and after nodes defer to padded-blocks and ignore cases when the checked node is at the beginning and end of a block, respectively.

If we keep this as two separate rules - one that enforces padding inside blocks and one that enforces padding outside blocks - could we not do the same thing? JSCS has both these rules - how did they handle cases where they conflict?

kaicataldo on 6 Nov 2016

Looking quickly at the 4 JSCS rules' source, seems there isn't any rules-conflict detection...
How is that possible? Does it allow an always-erroring config?

caesarsol on 6 Nov 2016

I believe JSCS relied on users to not create conflicting configurations.

TheSavior on 6 Nov 2016

1   class Component {
2       init () { // ^^^^ problem ??
3           some.code()
4       }
5   
6       render () {
7           return ''
8       }
9   } // ^^^^ problem?

lets say that the rules you haven't defined enforce padding before and after a block, and also enforce no padding at the beginning and end of the inside of a block. the "conflict" here happens between lines 1 and 2, and between lines 8 and 9. I don't think there is a need to address redundant padding between blocks since I think this can be intuitively collapsed much like margin collapsing in CSS. Sorry if this is redundant, just want to confirm that we're all on the same page with this and these things imo can be hard to discuss w/o example code..

quinn on 7 Nov 2016

@quinn, something we ran into with the JSCS rule are things like functions that are wrapped with bind, call, or apply. There should be a way to enforce a blank line after blocks wrapped in those, unless they come at the end of a parent block in which case there shouldn't be an extra blank line.

Does that make sense? Take a look at the jscs rule docs if not

TheSavior on 7 Nov 2016

I propose to ignore the cases in which the blank line would be inserted between lines of different indentation. To keep coherent terminology, the rule (or sub-rule) name would change from "outer padding" to "between padding".

(I say indentation for simplicity, the implementation should be based on scopes I think)

caesarsol on 7 Nov 2016

ok. that would be the same as using the prefer: "inner" config in the original example.

quinn on 7 Nov 2016

Yes, but with the advantage to make the rule splittable, I think.

caesarsol on 7 Nov 2016

yeah. The original reason for all of this was that it would be necessary to allow the user to choose how it was resolved rather than having it be resolved implicitly. Configurability seems preferable to me, but my specific usecase seems like it will be covered either way..

quinn on 8 Nov 2016

To be clear, would the following example be addressed by what is being proposed here?

1      var x;
2      if (x) {         // error because no line between var and conditional
3        doSomething(); // no error because this is within the new block
4      }
5      var c;           // error because no newline after previous block

As far as padded-blocks goes, if it's enforced, then that rule can just throw whatever error it wants if it's applicable. I'm pretty sure conflicting rules are possible right now, but I would call it user error if they are enforcing conflicting rules.

It would seem this logic would be quite similar to lines-around-comment

jensbodal on 9 Nov 2016

@jensbodal @caesarsol @TheSavior added a 2nd proposal that would add a 2nd rule and leave the existing one alone. it would also adds config options to prevent conflicts, and throws errors otherwise, assuming that having rules that can check other rules and throw errors based on that is even a thing.

quinn on 9 Nov 2016

I think we need some input from the eslint team on whether that is allowed. @kaicataldo @not-an-aardvark @nzakas

TheSavior on 9 Nov 2016

@quinn @TheSavior Rules are designed not to know anything about each other, so there's no way to "check for conflicts" unless it can be done without knowing the configuration of the other rule.

platinumazure on 9 Nov 2016

👍2

@platinumazure thanks. so the original plan of modifying padded-blocks seems the most reasonable?

quinn on 9 Nov 2016

would it be helpful if i were to hazard an implementation?

quinn on 9 Nov 2016

Just to make sure we're on the same page, could you clarify which plan you're referring to? It's great that there has been a lot of discussion on this, but I think I'm a bit lost regarding where the proposal stands at the moment.

not-an-aardvark on 9 Nov 2016

@not-an-aardvark this thing:

{
  inner: "always" | "never" | "before" | "after"
  outer: "always" | "never" | "before" | "after"
  prefer: "inner" | "outer"
  outerExceptions: "inCallExpressions" | "inNewExpressions" | "inArrayExpressions" | "inProperties" | "singleLine"
  innerExceptions: "blocks" | "classes" | "switches"
}

quinn on 9 Nov 2016

@not-an-aardvark that doesn't include your comments from earlier, but that's the general idea

quinn on 9 Nov 2016

Thanks for clarifying. My concern about starting to implement it right now would be that if other team members have some objection to the proposal, and we end up making changes to it before officially accepting the issue, then some the time you spend implementing this proposal might end up going to waste. But it's up to you.

not-an-aardvark on 9 Nov 2016

One of my concerns is that this rule proposal as part of the JSCS compat milestone has been talked about in different threads for months (or maybe it just seems that way). We've had an exceedingly large amount of discussion on this topic and have consolidated it into this issue. It seems like every time a new set of eyes comes in, they mention the same concerns, and weeks later (again, maybe it just feels that way) we end up in the same spot.

From my perspective it has been quite demotivating that it hasn't gotten enough attention to drive it in the correct direction and I'm frankly quite surprised that @quinn is still pushing on it as much as he is.

So, my goal isn't to rant, and I'm aware that I only am seeing one small part of what is happening in eslint land. So here are some more concrete questions:

What can we do as a group to push this forward. From my perspective it seems like we are just missing voices from the team to provide real constructive feedback on the proposal that has been created over the last few months.
What can we do so that future proposals don't end up in the same situation
What can we do so current and future contributors don't end up disenfranchised? Perhaps we should have written it as a plugin so we could have started getting the benefit of the rule while this conversation spiraled.

TheSavior on 9 Nov 2016

In order for this to move forward, we need a champion from the ESLint team who will drive this to conclusion -- that's really what's missing here.

I think I understand the rationale for one rule at this point. My only feedback on the proposal that the exceptions are really confusing as defined. We should either use node types ("CallExpression") or non-node types, but we shouldn't be mixing terms.

@TheSavior thanks for the feedback. The problem we're having right now is that we're under a crushing amount of issues and pull requests. We basically just can't keep up with all of the requests, and all of the conversations surrounding those requests; there just aren't enough people on the team (we've had a lot of inactive team members recently - including me, due to my health issues). So, we are stuck in a scarce resource problem on the one hand, and on the other hand, our popularity means we have to be extremely careful about introducing changes that could negatively affect the existing ecosystem (this issue is a good example of that potentiality).

I'll be the first to say that I wish we could resolve issues like this faster, and I commend @quinn for sticking it out with us.

To get back to how to move this forward: we need an ESLint team member to champion this change and drive it to its conclusion. Other suggestions for how to improve the process are welcome.

nzakas on 9 Nov 2016

@TheSavior Thanks for bringing this up. You make some fair points.

What can we do as a group to push this forward. From my perspective it seems like we are just missing voices from the team to provide real constructive feedback on the proposal that has been created over the last few months.

I think one of the reasons that progress has been moving slowly on this issue is because the proposal is relatively complex (since it has to account for cases that aren't immediately obvious), and there has also been a large quantity of discussion on the details. While neither of these are bad things (in fact, they're probably necessary for a proposal like this), they do tend to increase the amount of time needed to accept a proposal, because it's harder for team members to quickly look at the issue and :+1: it.

That said, I'm willing to champion this proposal. @eslint/eslint-team, this proposal is necessary for JSCS compatibility, and it's been in limbo for months; if we can get a few :+1:s on this issue, then we can finally get it implemented and added to core.

What can we do so that future proposals don't end up in the same situation

First of all, I acknowledge that we can probably do a better job of keeping track of discussions on issues to prevent them from getting stalled. That said, one thing that I've found to be useful is doing some of the bikeshedding for a proposal on Gitter, etc. This can reduce the time that it takes to get feedback on an issue, and it can also make it easier to follow the thread on GitHub, since the discussion that does end up on the thread ends up a bit more focused, which makes it easier for team members to look over and approve.

What can we do so current and future contributors don't end up disenfranchised? Perhaps we should have written it as a plugin so we could have started getting the benefit of the rule while this conversation spiraled.

You're certainly welcome to write a plugin for these compatibility issues. In terms of existing JSCS compatibility issues, I think one helpful thing to do is to create a concrete proposal that we can look at and vote on. For some JSCS compatibility issues, I personally agree that it should be somehow possible to lint for whatever the issue is talking about, but without a concrete proposal to look at, it's difficult to give other feedback. (Admittedly, this is only an issue for some proposals; others, like this one, do have a fairly concrete proposal.)

not-an-aardvark on 9 Nov 2016

From only being briefly familiar with eslint and all of the rules, my feeling is that some of the issues that this issue pertains to have been improperly amalgamated into extending an existing rule which satisfies two separate ideas. I simply want to add a _lines-around_ constraints to defined blocks. padded-blocks seems to address specifically the spacing within blocks, whereas I want to address spacing _around_ blocks. Therein lines the issue in which a block is considered where the { begins, whereas I am addressing the _keyword_ which defines the block.

I might also be struggling with the differences between padding and lines as to me they both seem to address the same thing. I might also be confusing a "block" with the keyword which defines a block versus the block itself.

E.g. in the following statement, the block could be considered to begin either with either for or {

for (var i = 0; i < len; i++) {
  // something
}

I was led here from this pull request, and I don't think that need is explicitly addressed here.

The rule I have in mind would be:

"lines-around-keyword": [
    2,
    "before": [
        "conditionalKeywords",
        "loopKeywords",
        "classKeywords",
        "variableKeywords"
    ],
    "after": [
        // same list or others ...
    ],
    "repeatingGroups": "variableKeywords"
],

Where conditionalKeywords would include protected keywords such as if & switch, loopKeywords would include for & while, variableKeywords would include var, const, let (etc...) and "repeatingGroup": variableKeywords would indicate that if keywords from one of the same groups listed are used then they would be ignored.

var a = 2;
var b = 3;   // valid because from same group
for (;;) {}  // invalid because from another group

/////////////////////////////

var a = 2;

for (;;) {}      // valid because not in group `variableKeywords`
for (::) {}      // invalid because not in `repeatingGroups` option
while (true) {}  // invalid because in group `loopKeywords`

//////////////////////////////

/* all valid */
var a;
var b;
var c;

for (var x in z) {
    var foo;
    var bar;

    while (true) {
        while(false) {
            var boo;
            var cat;
        }
    }
}

jensbodal on 10 Nov 2016

@jensbodal can you join on gitter? I like this a lot, and it fixes some things (e.g. newline-after-var) but i don't think it covers yet the original issue with the existing padded-blocks

quinn on 10 Nov 2016

I might have missed it in the multiple and lengthy discussions, but I am not sure why we can't delegate to padded-blocks when there is an inner block at the beginning or the end of the block. We do similar things in other rules.

It would also be helpful to try to stay consistent with the current padded-blocks schema.

And lastly, I am not sure if independent options for "before" and "after" are really needed even if they were part of the original JSCS rules.

alberto on 10 Nov 2016

@alberto this is essentially what is in "proposal 2" way back up the top of this which I crossed out.

https://github.com/eslint/eslint/issues/7356#issuecomment-259497115

@quinn @TheSavior Rules are designed not to know anything about each other, so there's no way to "check for conflicts" unless it can be done without knowing the configuration of the other rule.

I'm on gitter

quinn on 10 Nov 2016

@alberto also for your questions: the original jscs rule was only for "after" padding. it could be simplified to "around" padding, but that doesn't simplify any of the edge cases being discussed as far as i can tell.

as far as the schema, i'm assuming the details like that will be hammered out once consensus is reached

quinn on 10 Nov 2016

@quinn rules should not conflict or depend or other rule configuration, but they can leave some cases to other rules.

alberto on 10 Nov 2016

hey everybody. there was some back-and-forth about how to deal with this. given the current assumption that any rule can't be aware or know about any other configuration it seemed like trying to keep things separate would create the need for a lot of weird config options (e.g. "exceptAfterVar", "ExceptAfterDirective", etc to allow those rules to work) so it instead it seemed similar to just combine any rule that had to do with inserting newlines around things into a single rule.

{
    ifStatement: true,
    return: true,
    functionDeclaration: true,
    directive: true,
    varDeclartion: { before: true, after: false }
},
{
    exceptions: /* inherited from jscs */
}

the first object specifies which things you want padding around. The 2nd would be the original exceptions from jscs. if there's any ambiguity in the rules (I want newlines after my vars BUT NEVER before my functions??!????) the implementation would use the order of the keys to determine which one takes precedence which would allow for all the existing functionality of the other rules but without any manual conflict resolution by using exceptions. Of course, this would mean the deprecation of 3 (or more?) other rules. yikes! But, it should also allow for coverage of a lot of the existing jscs rules related to padding as well.

padded-blocks would continue to work as it does. This new rule would only apply for things in the same scope / indentation (by extension of that it would also not insert newlines at the beginning or end of a file).

thanks @jensbodal for your post earlier, this is based on that.

(potentially) deprecated rules:

quinn on 12 Nov 2016

👎1 👍1

@quinn Could you please edit your post with a list of which ESLint rules would need to be deprecated and be replaced by your proposal? Thanks!

platinumazure on 12 Nov 2016

@platinumazure yep done

quinn on 12 Nov 2016

👍1

@alberto one of the difficulties I feel that lumping this in with padded-blocks would be what I mentioned earlier, a block is defined by the opening and closing braces, not necessarily by the keyword which creates the need for a block.

For example if I wanted to prevent var a; if (a) return; or even

var a; var b; var c;

There is no rule to prevent this (that I'm aware of), and there is no "block" to match on to prevent this from happening (one-var also does not address this). If we were to include this functionality into padded-blocks we would essentially be redefining what is considered a block.

jensbodal on 12 Nov 2016

@jensbodal actually this is no longer combined with padded-blocks. that rule can still handle padding the inside of a block. this isn't really about blocks specifically and more about "sections" and putting padding (or lack of padding) between sections in the code.

quinn on 12 Nov 2016

Ok I might have misunderstood the comment regarding delegating to padded-blocks.

jensbodal on 12 Nov 2016

@quinn

A few questions:

if there's any ambiguity in the rules (I want newlines after my vars BUT NEVER before my functions??!????) the implementation would use the order of the keys to determine which one takes precedence

I'm not seeing how this order is determined in your proposal. Can you elaborate on that? Did you mean for the first configuration option to be an array? And if so, won't we have an array of objects (containing a key with potentially another object)? Feels really verbose and error prone to me.

This feels like a lot of overhead on the configuration side to me. Speaking as a user of ESLint, I don't want to have to think about the arbitrary precedence order of all these node types when I'm writing my configuration. Having to think something along the lines of "newlines are enforced after variable declarations when they're folllowed by a, b, and c, but not x, y, and z" for each node type specified sounds pretty tedious/overwhelming.

Are there sane defaults so that I can turn this rule on and have it just work?
If there are defaults, how do I change one option and ensure the rule still knows the correct precedence order? Or would changing one option require rewriting all the options every time so that the rule knows which node type configurations should take precedence?
Finally, this proposal uses "key order" (I'm assuming in object in an array) to resolve conflicts. What happens if the conflict is for the same node type? For example, what if you have have [{ ifStatement: { before: true, after: false }}] and have the following conflict:

if (a) {
}
if (b) {
}

How would the above be resolved?

kaicataldo on 13 Nov 2016

@kaicataldo the assumption is that key order is preserved. I think this is true in most if not all scenarios ? My original syntax used arrays, e.g:

/* eslint padding: ["error", ["ifs", "directives", "classes", "returns"]] */

This works great if you only want to specify wrapping things in padding but becomes more complex (array elements need to be replaced with objects instead of strings) to configure when you start wanting to enforce padding around some things and a lack of padding around others.

I think that's an important thing to remember here: if you are asking to insert padding somewhere, but not have it somewhere else, you are almost guaranteed to have a conflict at some point when those two things are next to each other. The point of the resolution through key order (or array order) is that we're dealing with a complex formatting problem with no simple answer (as of yet..) so at least the issues can be resolved concisely in the existing config, and not have to worry about a bunch of additional options.

I feel very strongly that for the majority of users this won't be a problem because the majority of users won't want an odd pairing of inserting and excluding whitespace. I don't think in most case a user would have to know or care about the importance and precedence of ordering.

quinn on 13 Nov 2016

@kaicataldo to your last question "how would the above be resolved": key order could be used again in those scenarios.

quinn on 13 Nov 2016

How can we ensure key order in an object? And for he second comment, wouldn't it be the same key? Really confused here. Maybe I'm totally misunderstanding what you mean by "key order".

kaicataldo on 13 Nov 2016

key order is preserved when looping over an object. I know there are edge cases but I don't think any apply here. If it helps, refer to the array syntax instead. I can expand on that if it helps.

quinn on 13 Nov 2016

Ah okay, I understand now - thanks for the explanation :) I'm still of the mind that the API proposed here is confusing and not very user-friendly. I think I'm taking issue with both the scope of the rule (that it covers so many different, seemingly arbitrary cases) and the complexity of the configuration (particularly the precedence behavior). I understand the problem it's trying to solve, but it feels really complicated and hard to reason about to me.

Apologies if this has been discussed already, but is the only issue not covered by a padding-after-blocks rule the request for enforcing new lines before IfStatements? If so, could we implement a padding-after-blocks rule and a newline-before-if rule that has configuration options that allow it to be compatible with the other newline-after-* rules?

kaicataldo on 14 Nov 2016

@kaicataldo are you on gitter?

quinn on 14 Nov 2016

@kaicataldo The "lets just add a rule for padding-after-blocks and give it an exception for every possible rule it could conflict with" idea hasn't been really fleshed out. I'll give it a try :)

quinn on 14 Nov 2016

(i'm waiting on a thumbs up from @alberto) ;)

quinn on 14 Nov 2016

@quinn Sorry, was traveling today and responding to things on my phone. Happy to discuss on Gitter when we're both on sometime :) We all really appreciate you taking the lead and working so hard on this!

@kaicataldo The "lets just add a rule for padding-after-blocks and give it an exception for every possible rule it could conflict with" idea hasn't been really fleshed out. I'll give it a try :)

If we were to split this into two smaller rules - padding-after-blocks and newline-before-if-statement - do you think that would simplify things? Or are you of the mind, having looked at this a lot already, that this would be more difficult?

My two main concerns are JSCS parity and ease of use/user experience - I'm :+1: for whatever we can do to hit both those marks!

kaicataldo on 14 Nov 2016

@kaicataldo I posted something in gitter, it's not complete but it's based on this (2 new rules)

quinn on 14 Nov 2016

Thanks for the discussion in Gitter, @quinn!

My issues with the current proposal

My biggest concerns with the current proposal are the complexity of the configuration and the behavior the concept of "precedence" causes. For example, with this configuration:

{
    IfStatement: { before: true, after: false },
    ReturnStatement: { before: true, after: false },
    VariableDeclartion: { before: true, after: false }
}

the following would be enforced:

if (a) {
}
return b;

var a;

return b;

Alternative idea

Without throwing the previous proposal out, wanted to share another possible approach @quinn and I discussed in Gitter. What if the rule could be configured with "always"/"never" and an exceptions object? In the case where there is a conflict, if the conflict involves an exception, it would not warn. Examples:

Valid cases:

/* eslint padding: ["error", "always", except: {
  ifStatement: { after: true }
  returnStatement: { before: true }
}] */

function a() {
  if (b) {

  }
  var c;
  return;
}

function a() {
  var b;

  if (c) {

  }
  return;
}

/* eslint padding: ["error", "always", except: {
  ifStatement: true
}] */

function a() {
 'use strict';

  var b;
  if (c) {

  }
  var d;

  return;
}

function a() {
  var b;
  if (c) {

  }
  return;
}

This ends up flattening the precedence order - you have defaults, and exceptions take precedence over the defaults. I think this is easier to reason about and enforces more consistent styling, however in its current state it has the drawback of enforcing padded newlines around a LOT of cases by default.

Variations on the above

A variation on this idea that might solve the problem of having too many things enabled by default (please consider this at a high level - names are something I picked quickly as an example):

/* eslint padding: ["error", {
  prefer: "padded",
  padded: ["ReturnStatement", "VariableDeclaration"],
  notPadded: ["Directive", "IfStatement"]
}] */

or

/* eslint padding: ["error", {
  prefer: "padded",
  padded: { 
    ReturnStatement: true,
    VariableDeclaration: { before: true }
  },
  notPadded: { 
    Directive: { after: true },
    IfStatement: true
  }
}] */

A much more simplified version would be to not allow for both requiring and disallowing padded newlines (user would have to pick one or the other):

/* eslint padding: ["error", "always", ["ReturnStatement", "VariableDeclaration"]] */

Questions

Open questions I have about this:

Should exceptions simply ignore cases or enforce the opposite?
How do we deal with the rule enabling so many things by default? This is why I prefer smaller rules that enforce one thing, but since these could all conflict, I don't know if we have that luxury.
What does everyone else think about the concept of precedence? As my example above shows, I think it's hard to reason about/configure and leads to what feels like enforcing arbitrary rules.

kaicataldo on 14 Nov 2016

Thank you guys so much for working on this and iterating on these proposals. Taking a step back, I have a high level question.

In order to do the best most expected thing for our users, we avoid having conflicts between rules that could cause a configuration to be invalid. Due to that, we have come up with this really complicated rule and configuration schema to avoid those conflicts. Of all the rules I've seen or used, all the proposals we've come up with to avoid conflicts with other rules have the most complicated configuration of any of them.

Do we really still feel like this is the best thing for eslint's users?

TheSavior on 14 Nov 2016

@TheSavior This is a * full spec * trying to cover many weird user preferences. Consider a simpler example:

/* eslint padding: ["error", "always", ["ReturnStatement", "VariableDeclaration"]] */

compared to this:

newline-after-var: ["error", "always"]
newline-before-return: ["error", "always"]

The first one I think could be argued to be simpler (or at least the same?), it's also more robust (it'll ensure newlines around things without doing something weird) and it's also less prone to errors:

/* eslint newline-before-return: ["error"] */
/* eslint newline-after-var: ["error", "never"] */

function foo() {
  var bar = 1;
  return bar;
}

I agree that it does seem overly complex, but an elegant solution hasn't really come up yet, and there does seem to be some issues with the existing rules (such as the one above, and their inconsistent names: before, after, around)

quinn on 14 Nov 2016

By the way, I'm going to temporarily remove my assignment on this issue. This is not to say I'm going to stop participating in the discussion (I still definitely want us to decide on and implement a good proposal, and I'm committed to addressing this issue in one way or another). However, since it looks like we've gone back to discussing proposal ideas, I want to make sure to come up with an idea that people agree on first.

not-an-aardvark on 14 Nov 2016

👍1

In the example you provided:

/* eslint padding: ["error", "always", ["ReturnStatement", "VariableDeclaration"]] */

That would enforce this code, right?

function foo() {
  // blank line before var
  var bar = 1;
  // blank line after var and before return
  return bar;
  // blank line after return
}

If a user also had

/*eslint padded-blocks: ["error", "never"]*/

then we are back into conflicting configurations, right?

TheSavior on 15 Nov 2016

no, this rule only inserts newlines on the same level of indentation. So unless you had done something with padded-blocks the result would be:

function foo() {
  var bar = 1;
                        // <- adds or enforces newline here
  return bar;
}

quinn on 15 Nov 2016

and the thing about indentation for the beginning in end of the file. If it's the first or last line of something (including the file) this rule doesn't do anything.

quinn on 15 Nov 2016

ok, i should be more specific. the last line it does nothing for * after *, and for the first line it does nothing * before *

quinn on 15 Nov 2016

I'm not sure how to best move forward with this. We really need to figure out some workable solution for JSCS compatibility. @eslint/eslint-team Can we give this another try?

kaicataldo on 10 Jan 2017

Would there be conflicts if the rule is split like this:

padding inside blocks (applies only to blocks not containing nested blocks)
padding outside blocks (applies to all blocks)

jmullo on 27 Jan 2017

@jmullo I think the solution that was getting the most traction was basically the opposite of that: outer padding applies * except * in the case of the beginning or ending of a wrapping block. This seems to be in line with the way the majority of people format their code:

block {
    block {
        // code
    }

    block {
        // code
    }

    block {
        // code
    }
}

block {
    var code = code
}

quinn on 27 Jan 2017

@kaicataldo what sort of consensus is needed from @eslint/eslint-team in order to allow a first draft of an implementation?

quinn on 27 Jan 2017

Consensus means that we have a champion and support from 3 team members for the proposal. We advise people to wait for this so that work/time isn't wasted. Anyone is free to make a pull request without consensus being reached - there's just no guarantee that it will be used. That being said, sometimes a prototype can go a long way in getting discussion rolling :)

kaicataldo on 27 Jan 2017

@kaicataldo understood :) I can get started on a PR, but I think things need to be a little bit more consensus-y before that can happen? because there is the qustion of do we just do the simplest thing to get JSCS compatibility or try to do something larger to address all of the conversation here? I think the discussion surrounding the latter sorta ended with your last comment here: https://github.com/eslint/eslint/issues/7356#issuecomment-260399695 and as far as JSCS I think it's literally just the rule AddPaddingAfterBlock or something. it's something really basic.

quinn on 27 Jan 2017

@quinn I'm sorry to ask again, but could you please remind me what the current/latest proposal is? Is it Proposal 1 in the top post? Or something else? Or are we still trying to figure that out?

platinumazure on 27 Jan 2017

I'm also a bit confused about the current proposal. I actually ended up here because I'd want beginning/end configuration options for padded-blocks, so that this would be possible:

block {
    // padding at the beginning
    block {
        // code
    }
} // no padding at the end

Maybe also so that you could have padding only for multiline code blocks and no padding for single line code blocks.

jmullo on 30 Jan 2017

👍1

@jmullo We're discussing padding outside of blocks in this issue. Feel free to make an issue for enhancing padded-blocks beginning/end of block configuration.

kaicataldo on 6 Feb 2017

Seems like what we keep getting stuck on is how to handle situations where there is a conflict.

I wonder if making the scope of the rule smaller by only enforcing newlines after blocks (which is what I believe the original JSCS rule did) would help in this regard? I started by advocating for the opposite, but it seems like it might be too challenging without having a way for rule configs to know about each other.

If we're only checking for newlines at the end of blocks, it seems to me like we can potentially even break these down into individual rules (newline-after-methods, newline-after-if-else, etc.).

This would leave potential conflicts only with lines-around-comments, lines-around-directives, and newline-before-return, and padded-blocks (cases where newlines can be required before a block), and we can defer to those rules in those situations.

One final thought - and please do correct me if this is incorrect - but it seems like the vast majority of times I see these rules used they are used to enforce that padded newlines are required, rather than enforcing that they are disallowed. Another way to simplify this would be to make them binary rules - when on, they only enforce the presence of newlines, not the lack thereof. Just a thought.

@eslint/eslint-team if we can't figure this out in the next week or so I think it might be time for the TSC to try to hammer something out.

kaicataldo on 6 Feb 2017

👍1

@kaicataldo changing it to only enforce newlines (or not care) would decrease the complexity considerably. I think the only place where it's important to enforce the lack of newlines is the existing padded-blocks rule which is remaining separate, so maybe it would be an easy way to get things going to only work on enforcing adding newlines.

I also think it makes sense to break things out into individual rules. It would be nice to deprecate things that are inconsistent though, and focus on making all the rules follow a consistent format newline-after-*. The only exception is rules that apply to things that always come at the end of an indentation, and (I think?) the only rule for that is newline-before-return. So all of these rules would have to check for two things: 1. is it at the end of a "block" (referring to indentation), 2. Is it directly before a return (it would no longer have to check for being at the beginning of a block since these would only be "after" rules). as long as the inconsistent rules remain it will be necessary to defer to those rules in order to avoid conflicts.

also I'm going to repeat how resolution of conflicts works: it's not like these rules would have knowledge of configuration outside of themselves, instead they just do nothing if there is a case where another conflicting rule * could * be applied, regardless of whether it actually is.

quinn on 6 Feb 2017

@kaicataldo Agree. If someone needs something more complex than that, they can create a custom rule.

Not sure about splitting, we should try to not to replicate the same logic on every rule to avoid conflicts with the other rules you mentioned.

alberto on 6 Feb 2017

@alberto Fair enough on keeping all the checks in one rule :+1:. So it sounds like we have two people on board for a rule that only checks for padding newlines after nodes with blocks (newline-after-block, maybe?). I think it makes sense to be able to turn each node type on or off individually, if we do this. A few more thoughts:

Should this only be for nodes with blocks? Or should we combine newline-after-var with this? I can see arguments for either way - I do think it would simplify the configuration process to have all padding newlines after nodes to be configured in one rule.
> Another way to simplify this would be to make them binary rules - when on, they only enforce the presence of newlines, not the lack thereof.

After considering the above some more, I'm not entirely sure it's actually necessary. My motivation for bringing this up was that it could simplify interactions with the other rules we outlined above that may require padding newlines before nodes. That being said, if we ignore nodes that are followed by a comment or a ReturnStatement (directives actually shouldn't be an issue since they'll always be at the top of a scope and already defer to padded-blocks when they are in a function body), I don't think this will actually be necessary. Two exceptions feels reasonable to me!

kaicataldo on 7 Feb 2017

not sure. once we go that route we'll probably end up with a config for every node type. do we want that?
enforcing never would create conflicts with padded-blocks:always and nesting.

@eslint/eslint-team as mentioned in last TSC, we need feedback on this one to move it forward.

alberto on 8 Feb 2017

response to item 2: @alberto one of the assumptions here is that padded-blocks always takes priority, meaning this rule / these rules never ever do anything at the beginning or end of a block or file. Much like how we are discussing these rules would also shut down if the next line was a return or a comment. It doesn't matter what the user's preference is, it always does nothing to avoid any potential conflicts.

response to item 1: "a config for every node type" has definitely and repeatedly been proposed in a multitude of formats.... and the original JSCS rule allowed for customization based on some node types (switch/case, class, etc.. can't remember exactly) but, the cleanest / sanest thing seems to be to create consistency, right? either have one rule with many config options, or to have many rules with consistent naming and parameters. Right now you have neither. :(

quinn on 8 Feb 2017

@alberto @kaicataldo wanted to continue with the padded-blocks thing.. I think the * only * thing that currently has really solid consensus is the thing about about interaction with padded-blocks: meaning that the new rule(s) do nothing when applying themselves to the beginning or ending of a block. It's been thoroughly discussed and I think everyone has agreed that it's the way to go (there are plenty of examples in the 15 gazillion comments above). So, even though there is still a lot to be worked out, I'd like to at least establish some portion of this stuff as having reached consensus.. it should streamline things, even if by only a small amount.. e.g:

[x] (CONSENSUS REACHED) new rule will do nothing when applying to the beginning or end of a block or file, to avoid ever conflicting with padded-blocks
[ ] (PRETTY CLOSE TO AGREEING ON IT) Rule(s) will apply to "after" only (rather than around or before)
[ ] (NO CONSENSUS, LEANING TOWARDS THE LATTER) should rules enforce whitespace only, or allow for enforcing of both (whitespace or lack thereof)
[ ] (NO CONSENSUS) should a single general newline rule be created, possibly deprecating other rules, or should new specific rules be created for each node type?
[ ] miscellaneous: option format (arrays or objects), removing other newline-* rules or avoiding conflicts with other rules, etc

quinn on 8 Feb 2017

It makes a reasonable amount of sense to me to only make this rule require white space, not disallowing it in others. This seems to get rid of any conflict issues but still allow a fair amount of expressivity. So you could say: ifStatement: both, var: after, returnStamement: before. Any other white space would be allowed (leaving the start and end of a block to padded-blocks I think we're all in agreement there). That way things like newline-before-return can naturally be expressed by this rule (and thus replaced).

Just my 2c.

aboyton on 9 Feb 2017

Thanks @quinn, you are right, I mixed the conflict with padding "before".

re item 1 (4th in your new list): it depends on whether we want to support every option in core. If we create a single rule, we certainly will end up there. I'm afraid it would explode in config, slowing this again, and implementation complexity (every node has its one-offs, newline-after-var is currently 250 lines long). Otherwise, we could focus on blocks, which seems to be the most demanded need. Thoughts?

@kaicataldo @quinn do you see any issue with enforcing lack of whitespace?

alberto on 11 Feb 2017

we could focus on blocks, which seems to be the most demanded need.

Do you mean only checking for newlines after BlockStatement nodes (with the exceptions we outlined above)? That could actually simplify this a lot!

We could always add more granular configuration in the future, since it would be opt-in and non-breaking. i.e. being able to turn the rule on/off for different parent node types (FunctionDeclaration, IfStatement, etc.). The only extra exception I think we should make now is that the rule should not warn for consecutive if/else if/else blocks.

I actually think this might be the best way forward.

do you see any issue with enforcing lack of whitespace?

I don't think there would be any issues if we're only checking after blocks and defer to other rules like newline-before-return when those cases occur.

So, here's what I'm now envisioning:

The following would error with /* newline-after-block: "error", "always" */:

if (a) {
}
if (a) {
}


a = {
  b() {
  },
  c() {
  }
}


class A {
  b() {
  }
  c() {
  }
}


function a() {
}
a();

The following would not error with /* newline-after-block: "error", "always" */:

if (a) {
}

if (a) {
}


if (a) {
}
else if (b) {
}
else {
}


a = {
  b() {
  },

  c() {
  }  // not an error because this will defer to `padded-blocks`
}


class A {
  b() {
  }

  c() {
  } // not an error because this will defer to `padded-blocks`
}


function a() {
}

a();


const a = function () {
}; // not an error because this will defer to `newline-after-var`
a();


if (a) {
  return a;
} // not an error because this will defer to `newline-before-return`
return b;

The following would error with /* newline-after-block: "error", "never" */:

if (a) {
}

if (a) {
}


a = {
  b() {
  },

  c() {
  }
}


class A {
  b() {
  }

  c() {
  }
}


function a() {
}

a();

The following would not error with /* newline-after-block: "error", "never" */:

if (a) {
}
if (a) {
}


if (a) {
}
else if (b) {
}
else {
}


a = {
  b() {
  },
  c() {
  } // not an error because this will defer to `newline-after-var`

}

class A {
  b() {
  }
  c() {
  } // not an error because this will defer to `newline-after-var`

}


function a() {
}
a();


const a = function () {
}; // not an error because this will defer to `newline-after-var`

a();


if (a) {
  return a;
} // not an error because this will defer to `newline-before-return`

return b;

Want to reiterate that we can always add more configuration later. Given the amount of discussion and time it's taken to get to this point, I really like the idea of targeting all BlockStatement nodes with the one "always" or "never" option to begin with. Adding the ability to turn on/off the rule for specific parent nodes should be pretty trivial once this lands (and will be non-breaking, since they will all result in fewer errors).

The other thing I like about this is that I think this most accurately enforces the concept of newline-after-block. I had been thinking of blocks in the context of their parent nodes before, rather than starting with blocks and thinking about the parent nodes only for configuration reasons. This seems like a much clearer approach.

So, in the future, configuration could look something like:

newline-after-block: "error", "always" or {
    FunctionDeclaration: "always",
    IfStatement: "never"
}

Thoughts?

kaicataldo on 12 Feb 2017

Do you mean only checking for newlines after BlockStatement nodes (with the exceptions we outlined above)? That could actually simplify this a lot!

Yes. I would also include classes and switches to go in line with padded-blocks. One possible controversy is that it also means it would ignore blockless statements (if, for, do, while) like:

if (a) foo();
bar();


if (a)
  foo();
bar();

alberto on 13 Feb 2017

I think not enforcing newlines around blockless statements would be expected in this case. Another option is for us to split out IfStatements into their own rule - newline-after-if - because it's likely to have its own configuration options (how to handle consecutive if statements, what do with block vs blockless statements, etc.), while keeping the rest.

Edit: @alberto Sorry, missed the part about the loops too. I still like the idea of this rule only handling blocks (only checking the BlockStatement's parent node when necessary). Perhaps we could have two rules - newline-after-block and newline-after-statment or newline-after-blockless-statement?

kaicataldo on 13 Feb 2017

In one of your examples where the following would pass with this configuration: /* newline-after-block: "error", "always" */

if (a) {
  return a;
} // not an error because this will defer to `newline-before-return`
return b;

What configuration would I need to enforce the following being okay?

function foo() {
  2+2;
  if (true) {
  } // Must have a new line after blocks

  3+3;
}

function bar() {
  2+2;
  if (true) {
  } // Must have a new line after blocks, including when it is followed by a return

  return 3+3;
}

function baz() {
  2+2;
  return 3+3; // But I don't *always* need a blank line before return. Only when it comes after a block
}

TheSavior on 13 Feb 2017

@TheSavior I would expect that to be handled by the newline-before-return rule and not be covered by the new rule we're discussing here. Feel free to make an issue for an option like that (only requiring the newline when it is following a BlockStatement) if you would like.

kaicataldo on 13 Feb 2017

Got it. Just wanted to make sure the logic for this rule had a plan for some of those edge cases and I wasn't sure how newline-before-return would handle that case. It seems to me like that approach would work.

TheSavior on 13 Feb 2017

@TheSavior I think in the above your middle example would be un-enforceable, because of the edge-case of specifying newline-before-return: never?

quinn on 13 Feb 2017

This is an eslint feature that I have been looking forward to since 9 months. I hope this rigorous constructive debate will reach consensus soon :)

sagiavinash on 13 Feb 2017

@quinn This new rule will have exceptions for when the block is followed by a ReturnStatement, so it's up to the newline-before-return rule in that case.

Thoughts on my latest proposal? I'd really like to try to finally reach consensus and get this implemented. Relevant comments:

https://github.com/eslint/eslint/issues/7356#issuecomment-279250710
https://github.com/eslint/eslint/issues/7356#issuecomment-279264438

kaicataldo on 13 Feb 2017

@kaicataldo I think what you are describing covers accurately the simplified version that we've been discussing in the last dozen-or-so comments... maybe it would be good to decide which things qualify as a "block"? imo, this is anything with indentation (any block, including any blockless statement broken out into more than one line). Would this rule also apply to blocks written on a single line, or blockless statements written on one line? and what about things indented that aren't necessarily blocks:

state = {
    default: true
}

func(
    has,
    many,
    args,
)

funcTakesObject({
    option: true
})

given that the above "blocks" didn't conflict with any of the situations described so far (before a newline, etc) would this rule apply to these? Just want to be clear :) so far we've discussed mainly unambiguous things like a function declaration or a bracketed if statement

quinn on 13 Feb 2017

@quinn I think this rule should quite literally check BlockStatement nodes only. I don't think it should cover object literals or call expressions, as neither is a block.

The one issue I see remaining is that this rule covers two kinds of blocks - cases where a BlockStatement is required (function declarations/expressions, methods) and cases where they aren't (if statements, loops).

We do need to discuss how to handle blockless statements - curious what others think of my proposal for a separate rule for that? Or at least agreeing that it doesn't belong in this rule? I personally think it would even be fine if we just did the newline-after-block rule now and revisit the blockless statement cases later if there are requests for such a rule from the community (as a separate rule).

I think trying to cover blockless statements statements in this rule muddles things, could make the configuration complicated, is kind of confusing given the name of the rule, and would hate to see it block this from landing.

kaicataldo on 13 Feb 2017

Since we ended up pretty much at the original purpose of the JSCS rule, I'd stick to that. See the exceptions there:
http://jscs.info/rule/requirePaddingNewLinesAfterBlocks.html

To reiterate, I'd use the same node types as in padded-blocks for consistency: BlockStatement, SwitchStatement, ClassBody.

alberto on 13 Feb 2017

👍1

Also important to note from the JSCS rule is that wrapping the block in a bind or a function in a call chain is fine too:

https://github.com/jscs-dev/node-jscs/blob/master/test/specs/rules/require-padding-newlines-after-blocks.js#L148

[].map(function(){}) // blank line not enforced after this
.filter(function(){})

function foo() {
}.bind(null) // blank line enforced after this. bind is not required to have a blank line above it.

2 + 2;

TheSavior on 13 Feb 2017

👍1

@kaicataldo my 2c, and the last time I will bring this up: whether a node is technically called a "block" in the AST seems to me to be irrelevant. since this is a code formatting rule i think the presentation of the code is more important. Meaning this is meant for sections delineated through indentation rather than any other criteria. this would pick up @TheSavior 's examples automatically, for example, and remove the need for a separate "blockless" rule (or any other rule for that matter). the node type seems like an irrelevant implementation detail to me, I think it's common to format a given node type differently in different scenarios, while still having very consistent whitespace.

quinn on 13 Feb 2017

@quinn

Meaning this is meant for sections delineated through indentation rather than any other criteria.

I disagree that we should tie the concept of blocks to indentation. There are many styles for indentation (as you can see from the indentation rule and all of its related bugs). I understand the desire to include statements in their "blockless" form, but this covering call expressions and object literals (and array literals, I suppose?) feels arbitrary and unrelated to blocks. I'm also really wary of constructing our own definition of what constitutes a block, as I think this just creates more overhead for the person configuring their linter.

For instance, should the following have a newline after if the rule is configured "error", "always"?

if (a) { return a; };

I also don't think that moving everything into one rule is necessarily a benefit. My preference is for smaller, simpler rules, as I actually think that these are generally easier to understand from a user perspective and easier to maintain on the maintainer side.

Finally, given how long this has been in discussion, I think we need to prioritize JSCS compatibility. If we can agree on a simplified version that gives us that, we can implement and then move onto a discussion for how we want to handle the "blockless" statements (whether that's in a new rule or an enhancement behind a configuration option in newline-after-block).

kaicataldo on 13 Feb 2017

@kaicataldo Sounds good to me :) thanks for taking the time to reason this out.

to answer your question, no, it shouldn't have a newline enforced. But I think that's going against your latest proposal, right? So for the sake of simplicity I'm assuming it would now insert a newline after a block, even if the block is on a single line. the JSCS rule has an exception for this scenario, i'm assuming we'll want to port that over as well

quinn on 13 Feb 2017

Thanks for your feedback @quinn. We'll keep JSCS compatibility.

It looks like we have reached some agreement, and this is a long thread, so I suggest we create a new one with the formal rule proposal and close this one. @kaicataldo do you want to create completing the missing bits from your proposal?

alberto on 13 Feb 2017

@alberto Sure thing - I'll try to get to it tonight.
@quinn Thanks for sticking with us and for all the thought you've put into this - your help has been invaluable!

kaicataldo on 13 Feb 2017

❤2

I'm sorry for late.
I know I'm a late arrival, but may I describe my proposal here?

https://gist.github.com/mysticatea/1b74f00c4b88aa45767226ce63b4a524#file-newline-between-statements-md

This is a abstract rule for linebreaks between statements, just it complements padded-blocks:

{
    // padded-blocks
    foo()
    // newline-between-statements
    bar()
    // newline-between-statements
    {
        // padded-blocks
        foo()
        // newline-between-statements
        bar()
        // padded-blocks
    }
    // newline-between-statements
    baz()
    // padded-blocks
}
// eol-last

The point of this rule is that this rule focuses "between 2 statements" rather than "before/after a statement".

This rule has flexible configuration, so it can replace newline-after-var and newline-before-return, it's more controllable of conflicting. At the same time, that implementation would be simple.

One of cons is that users may be relatively hard to configure it.

Thought?

mysticatea on 14 Feb 2017

I've probably missed something but I can't see @kaicataldo how you could later extend this rule by having

newline-after-block: "error", "always" or {
    FunctionDeclaration: "always",
    IfStatement: "never"
}

without conflicting with rules like newline-before-return. This is why I suggested we instead make the rule only enforce the presence never the absence of newlines. That or am I missing something and are conflicts not something we have to worry about?

Unless I'm missing something (and I probably am) the only way to not have conflicts is to have a config more like

newline-after-block: [FunctionDeclaration, IfStatement],
newline-before-block: [ReturnStatement]

aboyton on 14 Feb 2017

@mysticatea As you said, this rule will be hard to configure. The fact that configuring the rule would require knowledge of the different AST node types is a smell of that.

To me, it's an example of an advanced rule that could also exist as custom rule for people wanting total control over line padding, but I would also like to hear more opinions.

alberto on 15 Feb 2017

Had a response written out on my phone and lost it - sorry!

@mysticatea I have the same concern as @alberto about the configuration being complex and requiring knowledge of the AST. That adds a lot of overhead to configuring a pretty universal rule, and I don't think it's necessarily fair to force people to have to learn this stuff to use the rule. I'm also curious about some node type-specific quirks, like not requiring a newline before a ReturnStatement if it's the only node in a block.

I do like the concept of the proposal, though. Is there any way we could make the configuration simpler/easier to understand for someone who knows JS but might not have prior experience with JS ASTs? I think I could support this proposal if we can find a way to do so, as it does give more control.

@aboyton The reason we can extend this is that we discussed deferring to other rules (padded-blocks, newline-before-return, and lines-around-comment, and newline-after-var) when those situations are encountered. There's precedence for this in our other rules. @mysticatea's proposal would combine all these into one rule so we wouldn't need to do that.

kaicataldo on 15 Feb 2017

I agree that it would be relatively hard to configure.

But I think it's hard as well if somebody configures multiple rules for newline carefully as avoiding conflictions. "Rules for newline after statement" and "rules for newline before statement" are conflictable essentially. We would need to set priority but the priority would be unclear and not controllable. Or we would need to add options but it might get the combinatorial explosion.

On the other hand, newline-between-statements would not conflict any rules (If we deprecate some existing rules) and it has "use the last matched setting" rule.

Also, newline-between-statements uses only statement types, it's fewer than expression node types. I think people can image what is it (except Export*Declaration).

ExpressionStatement
BlockStatement
EmptyStatement
DebuggerStatement
WithStatement
ReturnStatement
LabeledStatement
BreakStatement
ContinueStatement
IfStatement
SwitchStatement
ThrowStatement
TryStatement
WhileStatement
DoWhileStatement
ForStatement
ForInStatement
ForOfStatement
FunctionDeclaration
ClassDeclaration
VariableDeclaration
ImportDeclaration
ExportNamedDeclaration
ExportDefaultDeclaration
ExportAllDeclaration

In addition, we have some rules which use AST node types already. E.g. no-multi-spaces.

I'm also curious about some node type-specific quirks, like not requiring a newline before a ReturnStatement if it's the only node in a block.

Because newline-between-statements checks newline between 2 statements, it does nothing if only one statement exists in a block. padded-blocks should check newline around the statement.

if (a) {
    // padded-blocks
    return
    // padded-blocks
}
// newline-between-statements
if (b) {
    // padded-blocks
    foo()
    // newline-between-statements
    return
    // padded-blocks
}

However, yes, my proposal has 2 special types: } and directive.
The } is the type of statements that the last token is }, to express newline-after-block.
The directive is the type of directive prologue, to express newline-around-directive.
Other special cases might exist...

mysticatea on 15 Feb 2017

The strategy we've been discussing to avoid conflicts is to defer to rules that enforce newline-before-* behavior (i.e. don't warn if the node being checked is before a ReturnStatement). That being said, I agree that this isn't ideal either - it then requires that you use those other rules in conjuction with the one being proposed here. Also, if we go that route we'll also have to add exceptions if we ever add more newline-before-* or newline-around-* rules.

Also, newline-between-statements uses only statement types, it's fewer than expression node types. I think people can image what is it (except Export*Declaration).

I agree that most of these names are self-explanatory. Maybe it wouldn't be too much overhead?

I really do like the concept of changing this from looking at nodes individually to looking at statements in pairs.

kaicataldo on 15 Feb 2017

@kaicataldo @mysticatea the newline-between-statements seems similar to https://github.com/eslint/eslint/issues/7356#issuecomment-259622085 made by @jensbodal. basically you group similar things. A return is a "thing", thus it gets a newline "around" itself (no need to have newline-before-return). right? Whatever solution is chosen, the existing newline-(before|after|around)-* rules should probably be dumped

quinn on 15 Feb 2017

@quinn

@mysticatea's proposal changes the original concept of enforcing padding lines around nodes to checking between nodes - so you define relationships, with priority being set by the order. If we go this route, we would be able to deprecate newline-after-var, lines-around-directive, and newline-before-return, leaving exceptions for lines-around-comment and padded-blocks.

The proposal I outlined above would require that the existing rules be kept.

@eslint/eslint-team Do we have any other opinions on this? My goal here is implementation and JSCS compatibility - let's try to decide on something! For reference, the two proposals on the table right now are https://github.com/eslint/eslint/issues/7356#issuecomment-279250710 and https://github.com/eslint/eslint/issues/7356#issuecomment-279585090.

I'm personally leaning toward @mysticatea's suggestion right now - my biggest concern is configuration complexity.

kaicataldo on 15 Feb 2017

oh, neat. I like @mysticatea 's idea a lot !

quinn on 15 Feb 2017

@mysticatea can you show how you would configure the exceptions from the original JSCS rule?

alberto on 16 Feb 2017

TSC Summary: requirePaddingNewLineAfterBlocks is a JSCS rule and one that is often requested.

There are currently two proposals:

newline-after-blocks that is a more or less straightforward port of the original rule, also including classes and switches. It would avoid conflicting with other padding rules by ignoring certain situations (e.g when next statement is a return statement).

newline-between-statements which is a more general rule that deals with padding between statements. It would replace newline-before-return, newline-after-var and lines-around-directive.

TSC Question: Which rule should we accept, if any?

alberto on 16 Feb 2017

@alberto

Ah, JSCS requirePaddingNewLinesAfterBlocks rule is checking newline about blocks inside of object literals, array literals, and call expressions as well. But my proposal newline-between-statements does not check those, so it's as same as "allExcept": ["inCallExpressions", "inNewExpressions", "inArrayExpressions", "inProperties"] configuration.

I think newline rules of each place should check it, probably behind options, if it's needed. Because even if we make an independent rule, we need ignoreBlocks-like options to avoid confliction between the rule and newline rules of each place.

inProperties ... object-property-newline
inArrayExpressions ... array-element-newline
inCallExpressions / inNewExpressions ... call-parameter-newline or something like?

For "singleLine" exception, I can add block and multilineBlock instead of } into STATEMENT_TYPE of newline-between-statements.

@kaicataldo

Maybe we can use the keyword which is at the top of statements instead of AST node types.

debugger → DebuggerStatement
with → WithStatement
return → ReturnStatement
break → BreakStatement
continue → ContinueStatement
if → IfStatement
switch → SwitchStatement
throw → ThrowStatement
try → TryStatement
while → WhileStatement
do → DoWhileStatement
for → ForStatement, ForInStatement, ForOfStatement
function → FunctionDeclaration
class → ClassDeclaration
var, let, const → VariableDeclaration
import → ImportDeclaration
export → ExportNamedDeclaration, ExportDefaultDeclaration, ExportAllDeclaration
??? → ExpressionStatement, BlockStatement, EmptyStatement, LabeledStatement

mysticatea on 16 Feb 2017

@mysticatea I really like the direction your proposal is going. Could we support both cases (node types and keywords)? And what about groupings (e.g., maybe all Export*Declaration can have no blank lines in between but there must be a blank line surrounding the group)?

platinumazure on 16 Feb 2017

@platinumazure

Could we support both cases (node types and keywords)?

Sure.

And what about groupings (e.g., maybe all Export*Declaration can have no blank lines in between but there must be a blank line surrounding the group)?

It can realize with:

{
    "newline-between-statements": ["error", [
        ["blankline", "export", "*"],  // need blank lines after exports.
        ["always", "export", "export"] // no need blank lines between exports. This is prior to `["blankline", "export", "*"]`
    ]]
}

mysticatea on 16 Feb 2017

TSC resolution: We have decided to go with @mysticatea proposal.

@mysticatea are you championing this?

alberto on 16 Feb 2017

Also thanks to everyone who contributed in the discussion or with a PR and helped us flesh this out! special hat tip to @quinn @TheSavior @LinusU @ekmartin

We know it has been painfully slow, and we apologize for that

alberto on 16 Feb 2017

👍5 ❤4

While this configuration enables lots of flexibility, it's also going to put a higher priority on solving the problem about extending configurations: https://github.com/eslint/eslint/issues/7957

Since all of the rules are being consolidated, if, for example, airbnb implements this, nobody will be able to add more things without having to duplicate their config for this rule and thus not be able to stay in sync.

TheSavior on 7 Apr 2017

👍1

Eslint: add outer padding to rule padded-blocks and cover existing jscs functionality

Update: please refer to this pull request for the current status of this issue:

1st proposal: updating padded-blocks

2nd proposal

Most helpful comment

All 120 comments

My issues with the current proposal

Alternative idea

Variations on the above

Questions

Related issues