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:

https://github.com/eslint/eslint/pull/8099

1st proposal: updating padded-blocks

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

The above doesn’t yet include additional options for backwards compatibility.

related issues:

related / converging rules:

2nd proposal

no modifications to padded-blocks. new rule that has exceptions to allow padded-blocks to work, if there is a conflict an error is thrown (is this even a thing?)

"padding-around-blocks": true | "before" | "after", {
  "exceptions": "inCallExpressions" | "inNewExpressions" | 
                "inArrayExpressions" | "inProperties" | "singleLine" | 
                "afterOpeningBlock" | "beforeEndingBlock"
}

"afterOpeningBlock" and "beforeEndingBlock" specify whether the rule should apply to a block placed at the beginning or end of the inside of another block. If they are both specified, then padded-blocks should work as expected. If they are not specified, then the new rule could potentially result in an error depending how padded-blocks is configured.

About this issue

  • Original URL
  • State: closed
  • Created 8 years ago
  • Reactions: 7
  • Comments: 120 (62 by maintainers)

Commits related to this issue

Most upvoted comments

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 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!

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 @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.

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.

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;

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.

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.

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.

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.

@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

}

@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