Skip to content
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

`space-after-keywords` fails with Allman brace style #4149

Closed
wabain opened this Issue Oct 15, 2015 · 13 comments

Comments

Projects
None yet
5 participants
@wabain
Copy link

wabain commented Oct 15, 2015

This is similar to #3226 and has also been described in comments to #4006.

Using ESLint 1.6.0, the space-after-keywords rule gives an error on code with Allman style braces because it expects there to be whitespace which is not a newline between an else statement and a subsequent brace:

/* eslint space-after-keywords: [2, "always"]  brace-style: [2, "allman"] */

var x = 1;

if (x)
{
   //...
}
else
{
   //...
}

// 11:1 - Keyword "else" must not be followed by a newline. (space-after-keywords)

Something similar happens after a do in a do-while statement. From the discussion on #3226 and #4006 it seems like the right approach here would be for space-after-keywords to allow newlines in these contexts and leave the rest to the brace-style rule.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 15, 2015

Thanks for the issue! 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.

@eslintbot eslintbot added the triage label Oct 15, 2015

@ilyavolodin ilyavolodin added bug rule accepted and removed triage labels Oct 15, 2015

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Oct 15, 2015

Confirmed in online demo. Agree, I don't think space-after-keywords should check for newline (other then to suppress a warning for missing space).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 16, 2015

Working on this.

nzakas added a commit that referenced this issue Oct 16, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 16, 2015

I put together a PR for this, though I realize there might be some edge cases like function (where you never want a newline). Just want to be sure that's what we want before making the change.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 20, 2015

@ilyavolodin thoughts?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Oct 20, 2015

@nzakas Can you elaborate on function example? Do you mean something like this:

function
a() {
...
}

I don't think it's part of this rule's job to check for something like this. This rule should only care that a space exists after a given keyword. The only exception should be that if keyword is followed by line-break, space is not necessary.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 21, 2015

Yup, that's what I'm talking about. If we're okay with the rule not catching that, then we're good to go.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Oct 22, 2015

If I'm not mistaken, the example above will fail to parse anyways. Again, I think we are overloading this rule with functionality if we are trying to make it check anything other then existence of space after a keyword. Rest of problems will be captured by either parser or other rules.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 22, 2015

It seems to parse OK.

You're probably right that this rule is doing too much and it shouldn't check newlines. Removing that seems like a breaking change to me. What do you think?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Oct 22, 2015

Hmm... That's a good question. I think it might be breaking. I'm not sure how people are using this rule right now, so it's hard to say. Just to clarify, I think we should still check for new lines, but only to verify that space isn't necessary if the keyword is followed by new line.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 22, 2015

Also @mysticatea had proposed that we combine space-before-keywords and space-after-keywords into keyword-spacing, so maybe we just hold off on making changes here and do that?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Oct 22, 2015

Sure. That sounds fine to me.

@PhiLhoSoft

This comment has been minimized.

Copy link

PhiLhoSoft commented Nov 4, 2015

Thanks for the PR, @nzakas, at least I could apply it manually on my copy of ESLint... 😄
Indeed, this part of code seems not to belong to the rule.

nzakas added a commit that referenced this issue Dec 8, 2015

@nzakas nzakas closed this in d8a98eb Dec 8, 2015

nzakas added a commit that referenced this issue Dec 8, 2015

Merge pull request #4173 from eslint/issue4149
Fix: space-after-keyword newline check (fixes #4149)

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

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.