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

new rule: space before (certain) keywords #1631

Closed
dwelle opened this issue Jan 7, 2015 · 20 comments
Closed

new rule: space before (certain) keywords #1631

dwelle opened this issue Jan 7, 2015 · 20 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@dwelle
Copy link
Contributor

dwelle commented Jan 7, 2015

There's a rule for space-after-keywords, but it seems to me it enforces just one half of the same styleguide, because it doesn't require space before keywords like else, catch, and finally.

if ( a ) {} else{}; // space after `else` is enforced
if ( a ) {}else {}; // while space before `else` is not enforced

Therefore, we probably should have a rule like space-before-keywords.

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

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion rule Relates to ESLint's core rules enhancement This change enhances an existing feature of ESLint labels Jan 7, 2015
@nzakas
Copy link
Member

nzakas commented Jan 7, 2015

You're right, though might be better to have a single rule do both.

@dwelle
Copy link
Contributor Author

dwelle commented Jan 7, 2015

agreed

@michaelficarra
Copy link
Member

Just space-keywords.

@gcochard
Copy link
Contributor

gcochard commented Jan 7, 2015

Config for this should look something like:

"space-keywords" : [2, "before/after/both"]

@lo1tuma
Copy link
Member

lo1tuma commented Jan 7, 2015

Seems like this is only related to keywords after a block statement. I think a rule like space-after-blocks (similar to space-before-blocks) would make more sense and should be easier to implement.

@lo1tuma
Copy link
Member

lo1tuma commented Jan 7, 2015

@gcochard The config should also accept "always" and "never" for each option.

@michaelficarra
Copy link
Member

While the try body is represented by a BlockStatement in the SpiderMonkey AST, it is not a block (or a statement). I would avoid applying block rules to all instances of curly braces in the JS syntax.

@nzakas
Copy link
Member

nzakas commented Jan 7, 2015

@lo1tuma I'm not sold. Having space-keywords with a configuration like:

space-keywords: [2, {
   after: ["if"],
   before: ["catch"]
}

Seems like it would both make sense from a user perspective and be pretty easy to implement.

@lo1tuma
Copy link
Member

lo1tuma commented Jan 7, 2015

@nzakas Your suggested configuration doesn’t work because you have to specify "always" or "never" for each keyword. So it would look like this:

"space-keywords": [2, {
    "after": {
        "if": "always",
        "else": "never"
    },
    "before": {
        "if": "never",
        "else": "always"
    }
}

I don’t like such complicated configurations.

@michaelficarra: Thanks, good to know. Maybe brace-style would be the correct rule.

@nzakas
Copy link
Member

nzakas commented Jan 7, 2015

I don't think always/never is necessary. The ones you list should have spaces and the ones you don't shouldn't.

@lo1tuma
Copy link
Member

lo1tuma commented Jan 7, 2015

We have an edge case with the function keyword where the default should be "don’t care" instead of "always" or "never".

@nzakas
Copy link
Member

nzakas commented Jan 7, 2015

Yeah, function is weird. I'm wondering if that should be handled by space-function-name instead, to encapsulate all spacing related to function.

@atomantic
Copy link

+1 on this

@nzakas
Copy link
Member

nzakas commented May 7, 2015

Thanks, +1s don't actually change anything. This issue is marked as accepted, which means it will get done when someone claims it.

@ilyavolodin
Copy link
Member

If we want to remove space-after-keywords and replace it with space-keywords this issue should be marked as required for 1.0.0, since we want to avoid removing existing rules after 1.0.0

@nzakas
Copy link
Member

nzakas commented May 17, 2015

Or we just add space-before-keywords. We need to be careful about adding more things into the 1.0.0 milestone or we might never get there.

I'd say if someone wants to drive this starting now, fine, but if it sits, then it will be post 1.0.0

@mmrko
Copy link
Contributor

mmrko commented Aug 12, 2015

+1

@IanVS
Copy link
Member

IanVS commented Aug 14, 2015

@mmrko, would you like to make an attempt at a solution? If the rule is important to you, the best way to get it implemented is to jump in and try it out. :)

@mmrko
Copy link
Contributor

mmrko commented Aug 17, 2015

Okay, I'll take a stab at this :)

@mmrko
Copy link
Contributor

mmrko commented Aug 29, 2015

Related PR: #3576

@nzakas nzakas closed this as completed in bff3741 Aug 31, 2015
nzakas added a commit that referenced this issue Aug 31, 2015
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

9 participants