It would be cool to have something similar to JSCS's http://jscs.info/rule/requirePaddingNewLinesAfterBlocks.html
Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.
Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.
Reporting a bug? Please be sure to include:
eslint -v)Requesting a new rule? Please be sure to include:
Requesting a feature? Please be sure to include:
Including this information in your issue helps us to triage it and get you a response as quickly as possible.
Thanks!
@okonet Can provide more information? What kind of problem are you trying to solve? Otherwise I think we will close this issue.
@okonet waiting for your response.
Sorry for a delayed response. My suggestion was to solve a case when a new
line is required only after the first block like in this example:
for (var i = 0; i < 2; i++) {
if (true) {
return false;
}
continue;
}
AFAIK right now this rule will require a new line also before the if
keyword. Correct me if I'm wrong.
On Sat, Aug 29, 2015 at 1:02 AM, Gyandeep Singh [email protected]
wrote:
@okonet https://github.com/okonet waiting for your response.
—
Reply to this email directly or view it on GitHub
https://github.com/eslint/eslint/issues/3092#issuecomment-135898340.
With best regards,
Andrey Okonetchnikov
So basically I'd like to have a padding rule _after_ a block. Current rule
only enforces padding _inside_ blocks.
On Thu, Sep 3, 2015 at 10:59 AM, Andrey Okonetchnikov <
[email protected]> wrote:
Sorry for a delayed response. My suggestion was to solve a case when a new
line is required only after the first block like in this example:for (var i = 0; i < 2; i++) { if (true) { return false; } continue; }AFAIK right now this rule will require a new line also before the
if
keyword. Correct me if I'm wrong.On Sat, Aug 29, 2015 at 1:02 AM, Gyandeep Singh [email protected]
wrote:@okonet https://github.com/okonet waiting for your response.
—
Reply to this email directly or view it on GitHub
https://github.com/eslint/eslint/issues/3092#issuecomment-135898340.
With best regards,
Andrey Okonetchnikov
With best regards,
Andrey Okonetchnikov
ping @eslint/eslint-team
✋ I would use this. I wouldn't think it would be a terribly involved implementation either.
:+1: I would love to see option to have padded blocks at the beginning only as it can be messy with longer line of code, however end of block is usually just a closing bracket, nothing noisy in there.
Should this be part of this rule or a new one? Given the current options names I think it might be confusing. We should also clear up if we want to allow padding before and after, and what would happen when always or never are set for padded-blocks.
We can probably keep it in the same rule and just modify the options to something like:
padded-blocks: [2, {
outside: { before: "always", after: " never" },
inside: { top: "always", bottom: " never " }
}]
This would be perfect!
That makes sense.
However there will be conflicts we need to clear up.
function(foo) {
if (foo) {
bar();
}
}
Which should take precedence here? Or should we ignore outside when the block is the first or last in a parent?
Good point. I'd say outside is lowest priority, so it should be ignored whenever there's a conflict.
@nzakas modifying the rule schema to what you proposed would make this a breaking change. Are you ok with that?
You can make it backwards compatible so it accepts both the old and new options.
How can I configure the rule for the below code style?
define([
'module/foo',
'module/bar'
], function (foo, bar) {
function list() {
var names = [];
names.push(foo.name);
names.push(bar.name);
return names;
}
return {
list: list
};
});
@Alex1990: First of all, for separating var declaration there is the newline-after-var rule. Regarding this rule, I am afraid you need to first decide on your style. You have return names followed by closing bracket but then for other return you do not stick to that style. In case you want your code look like this, then this rule cannot help, it would be just in your way.
Thanks. The first line inside the function curly braces is newline if the function body is too long. This style, in my opinion, is more readable. So, I didn't stick the always or never style. Maybe I should close this rule.
We would like to enforce padding for ES6 classes. Due to our styleguide those are treated differently than other blocks, which shouldn't be padded: infektweb/eslint-config-infektweb#12
Is it foreseeable to get a padded-blocks option (or a separate rule if you like) for treating them separately?
@christianharke that's not really related to this issue, please open a new one.
I'm not sure keeping this in the same rule is a good idea anymore, given:
always and never even more confusing.We now also have newline-before-return, which is in a similar vein to the proposed rule.
@kaicataldo hmm, how does this relate to blocks?
@alberto do you have a counter proposal?
@nzakas What's the current proposal? Adding
outside: { before: "always", after: " never" },
inside: { top: "always", bottom: " never " }
as top level options and for every property? And what about the JSCS exceptions?
The current schema is:
{
"enum": ["always", "never"]
},
{
"type": "object",
"properties": {
"blocks": {
"enum": ["always", "never"]
},
"switches": {
"enum": ["always", "never"]
},
"classes": {
"enum": ["always", "never"]
}
},
"additionalProperties": false,
"minProperties": 1
}
I think maybe a different rule padding-after-blocks as a more direct port of the JSCS rule is preferable to account for the exceptions. Or padding-around-blocks if you want to consider the padding before them, although it would also need exceptions to avoid conflicting with other rules (this one, newline-after-var and lines-around-comment come to my mind)
@nzakas That's...not entirely clear to me either. I must have misread this, and was thinking about enforcing newlines/padding. Sorry - please ignore 😊
Oops, lost track of this. I think I agree with @alberto that a separate rule makes sense rather than trying to combine with an existing rule
We should think about how this relates to https://github.com/eslint/eslint/pull/5950 as well.
I think one of the main topics here is the desired level of customization for different block types.
Should the rule automatically include switches, classes and all block types, or should it be possible to toggle it on and off based on the type? You could also go further and allow customization based on what kind of block it is, i.e. if it's an if/for/while/class method etc.
Personally I'm not sure if the extra complexity is necessary in this scenario. I'm of the impression that a lot of the people who would want to enable something like this would want it enabled for everything that ends with a curly bracket - i.e. that the block type itself isn't very important.
I do think it makes sense to differ between padding before and after blocks though, as I'd guess that there's a lot of people that would say the following is okay:
const a = 5;
if (a > 5) {
b();
}
But still want to prevent this:
if (a > 5) {
b();
}
if (c > 10) {
d();
}
This is similar to what @mysticatea suggests in this comment - and is sort of what I've implemented in #6445.
My implementation adds the functionality to the existing padded-blocks rule, which I think is an okay direction if no further customization is wanted. If you do however want to differ between block types or similar I think moving it to its own rule makes sense, as not doing so would make padded-blocks overly complicated.
It looks like we lost track of this issue (with an open PR). As far as I can tell, there are two different approaches proposed to this change.
First one would create another configuration for this rule in the following format:
padded-blocks: [2, {
outside: { before: "always", after: " never" },
inside: { top: "always", bottom: " never " }
}]
This does not cover newer enhancements to this rule that allow different style for switches and classes (although I guess proposed configuration could be expended to support them as well).
Second proposal is to create a new rule padding-after-blocks. I'm not 100% sure how we would be able to do that without a breaking change, since padded-block already takes care of padding after blocks, and we would have to remove that functionality from this rule before adding new rule.
@eslint/eslint-team Thoughts on those two proposals?
@ilyavolodin Hmmm, padded-blocks doesn't handle padding after blocks, only inside. The only options are "always" and "never", and those both apply to _inside_ the blocks only. That's why I think a new rule is the right way to go.
@eslint/eslint-team We should probably revisit this as it is part of the JSCS compatibility milestone
Agreed. I think this needs to be a new rule.
What is the status of this issue?
I'm happy to help contribute to this to push it over the finish line. I wrote the JSCS rule this is discussing and this is one of the last rules left in my company's conversion from JSCS to eslint.
Let me give a different perspective on the original JSCS rule requirePaddingNewLinesAfterBlocks (I was the author). A simpler way to think about that rule is that it intends to enforce that every closing curly line should be followed by a blank line unless followed by another closing curly line.
It seems like one of the challenges of adding a padding-around-blocks rule is the implications of conflicts when adding padding before the blocks. newline-after-var and lines-around-comment for example, as @alberto said.
I've been reading through this thread and it seems like nobody has been putting forth a use case for a blank line before blocks. Most of the cases people might be able to think of actually make more sense as after rules. For example, perhaps https://github.com/eslint/eslint/issues/7033#issuecomment-244271371 would actually be solved better with newline-after-imports
I propose that we name this rule something generic but start with only the use case we can well articulate. We should definitely come up with something that won't keep us from being able to support more use cases. That will enable us to move forward with the JSCS compat while solving the problems we actually need to address.
I'd like to propose the following schema, based on @alberto's comment:
{
"enum": ["always", "never"]
},
{
"type": "object",
"properties": {
"CallExpression": {
"enum": ["always", "never"]
},
"NewExpression": {
"enum": ["always", "never"]
},
"ArrayExpression": {
"enum": ["always", "never"]
},
"properties": {
"enum": ["always", "never"]
},
"singleLine": {
"enum": ["always", "never"]
}
},
"additionalProperties": false,
"minProperties": 1
}
ex:
`padding-around-blocks`: [2, "always"]
`padding-around-blocks`: [2, "always", {
CallExpression: "never",
properties: "never",
singleLine: "never"
}]
This would mean that if we later add support for lines before blocks, the usage would change to:
`padding-around-blocks`: [2, "always", {
CallExpression: { before: "always", after: "never" }
properties: "always",
singleLine: "never"
}]
It _would_ mean that there would be a backwards incompatible change to the rule when we add support for lines before because a value of always would start enforcing more. Then the question comes down to either having that backwards incompatible change, or starting with a more specific rule like padding-after-blocks that becomes deprecated once someone comes up with a valuable use case for space before.
TL;DR
Based on what I've seen from the JSCS rules, I don't think padding before blocks makes sense or is valuable. Unless someone can come up with a convincing argument, I don't think we should block JSCS compat on that hypothetical.
I think having a rule called padding-around-blocks that only checks after the block is confusing.
My preference would be to fully implement padding-around-blocks right off the bat. It does seem like this issue is converging with https://github.com/eslint/eslint/issues/5982, as @TheSavior has pointed out, and we would be able to solve both issues of compatibility with JSCS. @eslint/eslint-team thoughts?
What is the standard eslint approach to handling the conflicts with other rules that will come when providing support for the blank lines before blocks?
For compatibility with padded-blocks, we've done things like not check newlines before the first statement in the block and after the last (ex: http://eslint.org/docs/rules/lines-around-directive#rule-details). I believe we have some cases of creating config options that allow rules to be used simultaneously as well, but don't have any examples off the top of my head.
Do you have a proposal for how we can set up this rule such that it doesn't conflict with others? I don't have enough context to really have a recommendation here.
@kaicataldo @TheSavior What are the scenarios of this rule plus padded-blocks?
pad before: yes / no
pad after: yes / no
pad inner before: yes / no
pad inner after: yes / no
pad inner before unless first thing is block: yes / no
pad inner after unless last thing is block: yes / no
so if you want to * not * pad inner, but yes pad outer, you would need the last two to resolve the conflict.
does that make sense ? ! ?
Yeah, that makes sense.
So the conflicts arise when people use:
pad before: never
pad inner before: always
and they have code like:
function foo() {
function bar() {
}
}
No matter whether they have the blank line or not, one part of the rule will complain. At configuration time, we can detect these bad configurations and notify the user of the conflict, pushing them to use something to the effect of:
pad before unless block: always / never
So essentially, we don't try and work with the conflict and instead tell the user that their configuration combination isn't supported and that they need to use a different configuration value.
given some rules like this:
padded-blocks: ['error', 'always' | 'never']
outer-padded-blocks: ['error', 'never' | 'always' | 'only-after' | 'only-before', indent: true (default) | false]
these would conflict:
/* eslint padded-blocks: ["error", "never"] */
/* eslint outer-padded-blocks: ["error", "always", {indent: true}]
but these would not:
/* eslint padded-blocks: ["error", "never"] */
/* eslint outer-padded-blocks: ["error", "always", {indent: false}]
also this would not conflict:
/* eslint padded-blocks: ["error", "always"] */
/* eslint outer-padded-blocks: ["error", "always"]
@TheSavior could that also work? this i think cuts down on complexity and allows the padded-blocks rule to remain unchanged.
the point of indent true or false would be to either invert or maintain the choice specified for outer-padded-blocks where the line of code is indented compared to the preceding (or following) line.
Can you elaborate more on indent? I'm not sure I follow the intended purpose.
I think I've been leaning towards this all being one rule since they are likely to be heavily related and people will be jumping between them.
So essentially, we don't try and work with the conflict and instead tell the user that their configuration combination isn't supported and that they need to use a different configuration value.
Hmm, this doesn't seem like a good solution to me. I want to be able to pick a configuration and not have the linter tell me later on that my config is self-contradictory.
Instead, if code like this is encountered:
function foo() {
function bar() {
}
}
The rule should just apply one of the config options (i.e. ensure that the config is followed for the outer block, and don't report any errors for the inner block config).
@not-an-aardvark So essentially we would be saying that one option takes precedence over the other?
If they are separate rules, then it seems like it would require one rule to check and read the configuration of another rule.
However, if they are new options to the same rule, then I think we would be fine. I think this approach would work.
@TheSavior @not-an-aardvark I agree that it seems weird to have rules that could conflict, but I'm having trouble envisioning how the options would be passed to a single "padded-blocks" rule. Given these examples are all valid with different configs:
{ // always pad inner
{ // block 1
}
{ // block 2 never pad outer (unless there is indentation)
}
}
{ // never pad inner
{ // block 1
}
{ // block 2 always pad outer (unless there is indentation)
}
}
{ // always pad inner
{ // block 1
}
{ // block 2 always pad outer
}
}
{ // never pad inner
{ // block 1
}
{ // block 2 never pad outer
}
}
what would there corresponding rule configs look like?
@quinn I'm having trouble understanding what "never/always pad inner" and "never/always pad outer" mean in this context. Could you clarify?
I'm also confused about how indentation is related. You sort of explained the indentation option in https://github.com/eslint/eslint/issues/3092#issuecomment-253011709, but I still don't understand -- could you provide examples of code that the indentation option would affect?
Actually, I understand what you mean by "inner" and "outer" based on rereading the discussion above, so never mind that explanation.
Basically, I think that if we encounter a conflict, we should pick a direction and stick with it.
/* eslint padded-blocks: [2, {"outside": {"before": "never"}, {"inside": {"before": "always"}}}] */
{
// <-- We should require an empty line here
{
}
}
I think creating a new rule for this will cause conflicts, and the rule shouldn't unexpectedly fail due to conflicts sometimes. (If we want to declare specific schemas invalid, then we can do that, but then they should fail immediately, rather than unexpectedly bailing out when it encounters code that causes a contradiction.
Combining this into one rule makes a lot of sense to me, both for ease of configuration/discoverability and also because it allows us to better deal with situations where the two rules might conflict. Not sure if we want to deprecate padded-blocks in favor of a new rule or enhance the current padded-blocks rule (we would have to not enforce padding around/outside the braces by default to avoid this being a breaking change).
Another idea for configuration is to have something along the lines of
{
opening: {
before: true,
after: true
},
closing: {
before: true,
after: true
}
}
So instead of grouping inner and outer, we group by location.
@kaicataldo ok, given your example, how would this be described:
blocksHaveOuterButNotInner {
blockOne {
lineofcode
lineofcode
}
blockTwo {
lineofcode
lineofcode
}
}
spaceBetweenBlocks {
lineofcode
lineofcode
}
It doesn't have opening after, and it * does * have opening before, but it doesn't in the case where opening before and opening after conflict. How is the conflict described?
Sorry, not sure I'm following the question. Could you show me the configuration and the where the conflict is? I'm not necessarily advocating for opening/closing at the moment - was just an idea that we can potentially think about this configuration differently.
@kaicataldo yeah i'll try to explain it better.. so using the existing padded-blocks this is valid:
/* eslint padded-blocks: ["error", "never"] */
blocksHaveOuterButNotInner {
blockOne {
lineofcode
lineofcode
}
blockTwo {
lineofcode
lineofcode
}
}
spaceBetweenBlocks {
lineofcode
lineofcode
}
moreBlocksWithSpace {
lineofcode
lineofcode
}
for me, personally, what I'm trying to do is enforce the spacing in between the blocks in the above example while still allowing for no padding on the inside of blocks. Using the example from @kaicataldo this could look like:
/* eslint padded-blocks: ["error", {opening: {before: true, after: false}, closing: {before: false, after: true} }] */
spaceBetweenBlocks { // before opening: true, after opening: false
lineofcode
lineofcode
} // before closing: false, after closing: true
moreBlocksWithSpace { // this works
lineofcode
lineofcode
} // this works
blocksHaveOuterButNotInner {
blockOne { // this block wants padding before given the rule, but it is opening a block, so it gets no padding. or does it, given the "before opening: true" semantics ??
lineofcode
lineofcode
}
blockTwo {
lineofcode
lineofcode
}
}
@kaicataldo fwiw I don't have an opinion on inner / outer vs opening / closing, I think this issue exists in either scenario. I think it boils basically down to these two edge cases:
class block with method blocks that have one space between them)finally, more thinking thoughts, I think most people like a line of whitespace after the closing brace of a block, * except * when followed by another closing brace:
method1 () {
}
method2 () {
}
} // this closes the class and is not preceded by a space
@quinn The situation you are stating is the _exact_ reason I wrote the original rule in JSCS. Check out the valid and invalid options there.
The challenge is that we are trying to merge that use case with padded-blocks while trying to avoid conflicts.
What is your proposal for handling the case you stated?
idk.
/* padded-blocks: ["error", "never", {outer: "always", prefer: "inner"}] */
prefer states whether you should use the inner or outer rule if they contradict?
@TheSavior Does that syntax make sense to you? I tried to make it an extension of the existing "padded-blocks" so it is backwards-compatible.
Also if the eslint folks think this is headed towards something merge-able I'd be happy to put together a PR for this.
If the consensus lands on that extending padded-blocks might be a good idea I'd be happy to update #6445 as well (which does this, but with above and below for outer padding).
@ekmartin cool. I think you are right that there should be a rule for blocks and not just for a specific type of block (e.g. class methods). I was thinking for the outer: option it could be "above" | "below" | "always" | "never" which would mean one less config option compared to your implementation. regardless, how does your implementation handle:
{ "inside": "never", "above": "always", "below": "always" }
when the inside of a block starts with another block?
Unfortunately, in order for this to satisfy the JSCS compatibility requirement, we need to support the same options that JSCS supports.
http://jscs.info/rule/requirePaddingNewLinesAfterBlocks
"requirePaddingNewLinesAfterBlocks": {
"allExcept": ["inCallExpressions", "inNewExpressions", "inArrayExpressions", "inProperties", "singleLine"]
}
@TheSavior I think the scope here is broader than the old jscs rule, that seems to only be for padding after a block, whereas this covers the padding before and after a block as well as the padding inside a block (and the places where the two conflict). It seems what the jscs rule has that this one doesn't is more granular control over the type of block the rules are supposed to apply to.
Is there a way to support all of it? Because simply supporting the semantics of "requirePaddingNewLinesAfterBlocks" doesn't cover everything and still doesn't resolve the areas where "requirePaddingNewLinesAfterBlocks" plus "padded-blocks" would conflict.
Yeah, we definitely are supporting a super set with our proposal to padded-block.
My comment was in reference to the type exceptions. Somehow we need to have answers for those options. Any recommendations?
@TheSavior I guess those aren't for the type of block it is, but rather where the block is.. it seems to mostly be for when a block is contained in some sort of list (function call, new, array, property list), those exceptions could probably just work as-is right? as in, use the padded-block rule unless the block is part of an args list, port of a property list, etc?
Yeah, that sounds right to me. Any recommendation on how that fits into the proposed padded-block configuration?
/* padded-blocks: ["error", "never", { outer: "always", prefer: "inner", outerExceptions: ["inCallExpressions"] }] */
?
e.g.
/* padded-blocks: ["error", "never", { outer: "always", prefer: "inner", outerExceptions: ["inCallExpressions"] }] */
blocksHaveOuterButNotInner {
blockOne {
lineofcode
lineofcode
}
blockTwo {
lineofcode
blocksonblocksonblocks {
return 'block'
}
lineofcode
}
}
spaceBetweenBlocks {
lineofcode
call(
arg1,
isAblock {
veryblocking
},
arg3
)
lineofcode
}
moreBlocksWithSpace {
lineofcode
lineofcode
}
Thanks for putting in this effort to come up with examples. I feel like it is really beneficial to moving this along.
So there would be outerExceptions and innerExceptions and they would just flip the other configuration for those cases? What is the first "never" for? It seems like in previous comments we have specified inner and outer with before and after.
the first never says "never pad the inside of a block". it's just doing what padded-blocks does currently. Then the outer: option says whether you want padding before, after, or both (always). prefer: states which takes precedence (inner or outer choice).
idk if there needs to be innerExceptions (maybe that is just for my uses), and I was thinking also for the exceptions it would just not do anything (rather than inverting the choices) since it would be hard to know exactly what inverting it should do..
@TheSavior ["error", "never"] could be considered the same as ["error", {inner: "never"}] for the sake of compatibility. i was thinking to just forego the latter and maintain the existing functionality without an alternative option that does the same thing.
I think having the alternative syntax is valuable for consistency. There is no rhyme or reason for the first one to inherently be "inner", it's just the case to maintain backwards compatibility. I'd imagine that over time that schema would be deprecated.
Do you feel like there are more outstanding questions?
We should write up something more comprehensive based on where we have landed and how different configurations would work.
A summary of this discussion and where the proposal currently stands would be really helpful! Could we create a new issue as requested here?
@TheSavior @kaicataldo wip here:
https://gist.github.com/quinn/ee7e4be90482c2b6517a8b090397e7f1
once it's hammered out we can create an issue? no point in starting yet another issue with 50+ comments....
Appreciate you pulling that together.
I disagree - I think part of the reason this discussion has stalled so many times is that it spans multiple issues and is hard to piece together. Creating a new issue to pull it all together and distill what we've discussed on the other issue would make it a lot easier for people to jump in. Discussing on the gist just makes it harder to find, imo. Then we can close the other issues and focus on the one. If we need to make a separate issue after that with a concrete proposal, that's fine - doesn't cost us anything to make a new one!
Would you like to make it? What you have in the gist seems like a fine starting point to me :+1:
@kaicataldo ok created issue: https://github.com/eslint/eslint/issues/7356
Thanks :+1:
Most helpful comment
We can probably keep it in the same rule and just modify the options to something like: