New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for switch statements in padded-blocks #5056

Closed
alberto opened this Issue Jan 24, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@alberto
Member

alberto commented Jan 24, 2016

This should be a problem:

/*eslint padded-blocks: [2, "always"] */

switch (a) {
  case 0: foo();
}

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 24, 2016

Member

A problem by default or a problem with an option?

Member

nzakas commented Jan 24, 2016

A problem by default or a problem with an option?

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Jan 24, 2016

Member

By default, IMO. Do you see any reason why it should be different from other blocks?
We already include them in block-spacing too.

Member

alberto commented Jan 24, 2016

By default, IMO. Do you see any reason why it should be different from other blocks?
We already include them in block-spacing too.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 25, 2016

Member

Just trying to think of any reason we might have omitted this initially, but can't come up with anything. I do wonder if people want to style switch separately, but I guess we will find that out.

Member

nzakas commented Jan 25, 2016

Just trying to think of any reason we might have omitted this initially, but can't come up with anything. I do wonder if people want to style switch separately, but I guess we will find that out.

@nzakas nzakas added accepted and removed evaluating labels Jan 25, 2016

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Jan 25, 2016

Member

I guess then we should also apply this rule for ClassBody.
On Jan 25, 2016 5:49 PM, "Nicholas C. Zakas" notifications@github.com
wrote:

Just trying to think of any reason we might have omitted this initially,
but can't come up with anything. I do wonder if people want to style
switch separately, but I guess we will find that out.


Reply to this email directly or view it on GitHub
#5056 (comment).

Member

lo1tuma commented Jan 25, 2016

I guess then we should also apply this rule for ClassBody.
On Jan 25, 2016 5:49 PM, "Nicholas C. Zakas" notifications@github.com
wrote:

Just trying to think of any reason we might have omitted this initially,
but can't come up with anything. I do wonder if people want to style
switch separately, but I guess we will find that out.


Reply to this email directly or view it on GitHub
#5056 (comment).

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Jan 26, 2016

Member

@lo1tuma Yeah I looked into that. The ClassBody body is already a BlockStatement. Same with parenthesized arrow function body.

Member

alberto commented Jan 26, 2016

@lo1tuma Yeah I looked into that. The ClassBody body is already a BlockStatement. Same with parenthesized arrow function body.

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Jan 26, 2016

Member

@alberto That's not a correct. A ClassBody node is not a BlockStatement node.

/*eslint padded-blocks: [2, "always"] */

class Foo {
    foo() {}
}

This doesn't report a problem in the online demo.

Member

lo1tuma commented Jan 26, 2016

@alberto That's not a correct. A ClassBody node is not a BlockStatement node.

/*eslint padded-blocks: [2, "always"] */

class Foo {
    foo() {}
}

This doesn't report a problem in the online demo.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Jan 26, 2016

Member

Oops, you are right. (I meant the body of ClassBody, but it's not a BlockStatement either)

Member

alberto commented Jan 26, 2016

Oops, you are right. (I meant the body of ClassBody, but it's not a BlockStatement either)

@btmills

This comment has been minimized.

Show comment
Hide comment
@btmills

btmills Jan 31, 2016

Member

I just made a proposal on #5092 that would add support for class bodies. I won't cross-post the entire proposal here, but tl;dr:

{
    "rules": {
        "padded-blocks": [2, {
            "blocks": "never",
            "classes": "always",
            "switches": "never"
        }]
    }
}

That's if you all think switches should be separate from blocks. If not, then that proposal is irrelevant.

Member

btmills commented Jan 31, 2016

I just made a proposal on #5092 that would add support for class bodies. I won't cross-post the entire proposal here, but tl;dr:

{
    "rules": {
        "padded-blocks": [2, {
            "blocks": "never",
            "classes": "always",
            "switches": "never"
        }]
    }
}

That's if you all think switches should be separate from blocks. If not, then that proposal is irrelevant.

alberto added a commit that referenced this issue Feb 5, 2016

alberto added a commit that referenced this issue Feb 7, 2016

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Feb 7, 2016

Member

Per the conversation in #5092, this would also be breaking, so the same decision should be applied here.

Member

alberto commented Feb 7, 2016

Per the conversation in #5092, this would also be breaking, so the same decision should be applied here.

alberto added a commit that referenced this issue Feb 16, 2016

alberto added a commit that referenced this issue Feb 17, 2016

alberto added a commit that referenced this issue Feb 17, 2016

alberto added a commit that referenced this issue Feb 17, 2016

ilyavolodin added a commit that referenced this issue Feb 25, 2016

Merge pull request #5162 from eslint/issue5056
Update: Support switch statements in padded-blocks (fixes #5056)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.