Option to tolerate single lines with padded blocks

[alphanull]

What rule do you want to change?

"padded-blocks"

Does this change cause the rule to produce more or fewer warnings?

fewer

How will the change be implemented? (New option, new default behavior, etc.)?

New option. Could be similar to { "allExcept": ["singleLine"] } in JSCS (http://jscs.info/rule/requirePaddingNewLinesAfterBlocks)

Please provide some example code that this change will affect:

if (cond) { doSomething(); }

What does the rule currently do for this code?

Warning / Error: "Block must be padded by blank lines."

What will the rule do after it's changed?

Emits no warning if using { "allExcept": ["singleLine"] } option

[eslintbot]

Hi @alphanull, thanks for the issue. It looks like there's not enough information for us to know how to help you.

If you're reporting a bug, please be sure to include:

1. The version of ESLint you are using (run eslint -v)
2. What you did (the source code and ESLint configuration)
3. The actual ESLint output complete with numbers
4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

Requesting a rule change? Please see Proposing a Rule Change for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

[btmills]

Since the link seems to be broken at the moment, here's our rule change proposal template.

[alphanull]

No problem, I have updated my initial post. Hope its OK now.

[btmills]

Thanks @alphanull. Since this is similar to a JSCS option, I've gone ahead and added it to the JSCS milestone.

JSCS has five possible exceptions for the requirePaddingNewLinesAfterBlocks rule:

• "inCallExpressions" Blocks don't need a line of padding in function argument lists
• "inNewExpressions" Blocks don't need a line of padding in constructor argument lists
• "inArrayExpressions" Blocks don't need a line of padding in arrays
• "inProperties" Blocks don't need a line of padding as object properties
• "singleLine" Blocks don't need a line of padding if they are on a single line

Is this something where we should add them all at once, or just as requested ("singleLine") in this case?

[alphanull]

"singleLine" would be what bothers me the most right now. But in this case other options could also be useful to other folks (and maybe even to me, not sure ATM), so why not add them all at once? Then it would also be fully compatible to JSCS. In addition, I would also like to see an option to specify minumum number of lines within a block, like described in #7146. And yes, adding options to enforce padding after and/or before blocks would also be nice (#6445). So it might look like padded-blocks is ready for an overhaul ;)

[platinumazure]

We just need to be careful about where we might be covering the other JSCS options in other ESLint rules. For example, "inProperties" might be covered by ESLint rule object-curly-newline.

[kaicataldo]

I wonder if, to get the ball rolling, we could just implement singleLine (which seems like the one that is most useful) now and wait and see if there are requests for the other options in the future. Since they're all behind their own unique flags and off by default, they can all be implemented independently and none are breaking changes.

I can champion this change if the above sounds good!

[alphanull]

Yeah, that would be great! I agree with singleLine being the most imporant thing to implement here.

[vitorbal]

@kaicataldo I think that's a good idea! 👍 from me.

[not-an-aardvark]

This seems like a good idea, but I don't think allExcept: ["singleLine"] is a good API for it. An array containing a single string seems like an inelegant API to me; I think it would be better to have explicit object options instead.

How about something like this?

rules:
  padded-blocks: [error, always, {allowSingleLineBlocks: true}]

[kaicataldo]

@not-an-aardvark Agreed. My proposal is to implement the feature, but wasn't necessarily intending the same API. Sorry, should have made that clearer!

[kaicataldo]

Do we want to continue pursuing this now or wait until we see how this discussion resolves?

[sscaff1]

Almost the same as #7775

[platinumazure]

Ah crap, I didn't do my due diligence correctly.

@sscaff1 @kaicataldo How should we handle this? Can this issue (or #7775) move forward by itself, or do we really need to tie this to #7356?

[sscaff1]

Either issue will solve it for me so it doesn't matter to me. I think either my issue or @alphanull issue can move forward by itself. My issue probably introduces a more flexible API although I'm unclear of what the use case is to require 3 lines for example before padding (although I'm sure it's out there somewhere).

[qlaire]

I'm going to work on this issue.

[ilyavolodin]

I know this has been accepted a while back, but I have a new proposal for this. How about deprecating existing blocks, classes and switches options, and adding exceptions option of type Array, that accepts ASTNode|"single"|"block"? That should cover all JSCS cases, and bring this rule inline with other rules we have.

[kaicataldo]

@ilyavolodin I think this makes sense, though I'm a little bit concerned that we've made a few changes that require the user to look up the ESTree node type to configure (example). I wonder if we should create some docs surrounding ASTs to make this more easily discoverable.

[ilyavolodin]

@kaicataldo Sure, that would be fine. We have a bunch of rules that already accept AST node types as parameters, so I think that would be a good practice to follow.

[kaicataldo]

@ilyavolodin Do you mind fleshing out your proposal? It sounds good to me, but I think it would be helpful for contributors to have a clearer picture of what's being asked for here.

@eslint/eslint-team Thoughts on this?

[kaicataldo]

@ilyavolodin I think the thing we need to hash out here is what an exception actually means. The current rule actually has three states when the object form is specified - padded, unpadded, and ignored (when not specified).

Were you thinking that exceptions would be ignored or would enforce the opposite of the first string configuration option?

[kaicataldo]

@eslint/eslint-team I'd really like to get this implemented. Given the complexities that are introduced by #7145 (comment) that I've outlined in my previous comment, I wonder if it would be beneficial to implement either the proposal in #7145 (comment) or to solve this with the proposal in #7775?

To reiterate, my goal here is JSCS compatibility. Once we have that, we can always iterate!

[ilyavolodin]

Really sorry, I totally lost track of this issue. My proposal was to change the options to always|never as a first argument, and an array of ASTNode|"single"|"block" as a second option. Anything in the array would be the opposite of the first value.

[nzakas]

@kaicataldo @ilyavolodin are either of you still interested in implementing this?

[kaicataldo]

Apologies for dropping the ball as the champion of this issue. Given that this is a JSCS compatibility issue, I would like to see this through.

While I appreciate @ilyavolodin's solution, I would like to propose we go with the previous solution as described here. We can always change the rule in the future if need be, and this would buy us JSCS compat without having to make major changes to the implementation.

If this is amenable to the team, I'm happy to make a PR myself.

[richie3366]

I wanted that option, so I implemented it myself (see PR #11243).

The tests & docs are not shipped in my PR, so it will need some contribution from you guys.

[kaicataldo]

I'm not entirely sure what to do here. There was initially consensus, and then a new approach was suggested, but it looks like the conversation has stalled. @ilyavolodin Any other thoughts on my questions above?

[ilyavolodin]

@kaicataldo I guess I'm fine with going with initial proposal, and maybe later implementing a simplified set of options that I suggested.

[richie3366]

@ilyavolodin And what about @not-an-aardvark proposal?

It introduces less breaking changes and it's already PR'd and ready to review (and merge if everyone agrees on this proposal & on the PR review).

Sorry if I may have done a wrong move by making a PR on a proposal which hadn't been agreed upon yet.

[ilyavolodin]

Ah, thanks. I forgot about that. Yes, I would prefer syntax suggested by @not-an-aardvark It's more inline with other rules we have.

[kaicataldo]

Sounds like we've come to a consensus and have accepted the implementation outlined here.