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:
- https://github.com/eslint/eslint/issues/3092
- https://github.com/eslint/eslint/issues/5982
- https://github.com/eslint/eslint/issues/5949
related / converging rules:
- http://jscs.info/rule/requirePaddingNewLinesAfterBlocks.html
- http://eslint.org/docs/rules/padded-blocks
- I think jscs has another rule for padding before blocks?
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
- New: newline-between-statements (fixes #7356) — committed to eslint/eslint by mysticatea 7 years ago
- New: newline-between-statements (fixes #7356) — committed to eslint/eslint by mysticatea 7 years ago
- New: padding-line-between-statements rule (fixes #7356) (#8099) * New: newline-between-statements (fixes #7356) * Chore: refactor with AST selectors * Fix: add more tests. - The kind `direct... — committed to eslint/eslint by mysticatea 7 years ago
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.
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
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
, andnewline-before-return
, andpadded-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:
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.
@platinumazure yep done
@nzakas The issue is that the two rules would conflict with each other:
@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