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

`keyword-spacing` from merged `space-before-keywords` and `space-after-keywords` #3869

Closed
mysticatea opened this Issue Sep 20, 2015 · 21 comments

Comments

Projects
None yet
5 participants
@mysticatea
Copy link
Member

mysticatea commented Sep 20, 2015

I thought the way of detecting keywords is the same in space-before-keywords and space-after-keywords, so I made an idea that merges two.
This rule is similar to semi-spacing, comma-spacing, and arrow-spacing.

{
    "keyword-spacing": [2, {"before": true, "after": true}]
}

Note:

Perhaps space-return-throw-case can be merged if there is an overrides option that is similar to operator-linebreak's.

Original rules are marked deprecated and will be removed at 2.0.0.


Could I get your opinion?

I'm happy to work on this if accepted.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 22, 2015

Are all the exceptions you list already implemented in the existing rules?

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Sep 23, 2015

@nzakas No, the first exception is not implemented. So space-before-keywords can be conflicting with semi-spacing, arrow-spacing, comma-spacing, key-spacing, and space-infix-ops (#3878) in the following patterns.

image

But I'm not 100% sure this exception is proper.

2nd and 3rd exceptions is the same behavior as the existing rules.
space-after-keywords is not checking spacing which is preceded by function, return, throw, case, new, delete, typeof, void, yield, and super.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 23, 2015

Can you explain the first exception some more? I'm not sure I understand what it would do.

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Sep 24, 2015

The first exception limits checking spacing to avoid conflicting with other spacing rules.
(I found missed pattern right now, so I modified a bit)

  • Only when the previous token is one of ), ], and }, it checks spacing between the previous token and a keyword.
if (a)var b = 0; // missing a space.

if (a) {
}else {} // missing a space.

// I cannot found any `]` pattern...

//--------

let a;function b() { } // semi-spacing.
(0,function() { }); // comma-spacing.
() =>function() { }; // arrow-spacing.
(function() { }()); // space-in-parens
a[this.b] = 0; // array-bracket-spacing
{function a() { }} // block-spacing
var a = 100 *this.b; // space-infix-ops
var a = {foo:function() { }}; // key-spacing
  • Only when the next token is one of (, [, and {, it checks spacing between the next token and a keyword. If space-return-throw-case is merged, there are additional tokens: !, ~, +, -, ++, and -- (these are unary operators).
if(a) { } // missing a space.
class A extends(b ? C : D) { } // missing a space.

class A extends[] { } // missing a space.

if (a) { }
else{ } // missing a space.
class A extends{name: "a"} { } // missing a space.

return(a + b);
return[];
return{a: 0};
return!foo;
return~foo;
return+foo;
return-foo;
return++foo;
return--foo; // missing a space (space-return-throw-case)

//------------

var a = this; // semi-spacing
var a = () => (foo(), this); // space-in-parens
var a = {b: 0, c: this}; // object-curly-spacing
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 24, 2015

Ah, I see. Thanks for explaining.

@eslint/eslint-team thoughts?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 12, 2015

@eslint/eslint-team we need to come to consensus on this proposal. Thoughts?

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Dec 12, 2015

Changing rules can be confusing for some folks, so without a bit more clear added value, I'm not overly enthused about it. Does this fit in with other efforts to combine rules? In general I've seen rules go the other way. Obviously a lot of thought has been put into it, so maybe there's more to it than that.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 12, 2015

👍 from me- I like how many of the other whitespace rules are symmetrical and I appreciate the thought that went into ensuring this new rule wouldn't conflict with existing rules.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 12, 2015

I don't see any problem with merging those two rules. Should be a bit easier to configure for new users.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 12, 2015

@xjamundx we've generally been merging space-before-* and space-after-* into *-spacing rules since people tend to configure both or neither.

If we want to do this, then it would be good to get into 2.0.0, otherwise we'd need to wait until 3.0.0.

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Dec 12, 2015

Makes sense 👍

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 14, 2015

And may I merge space-return-throw-case also?
returns, throw, and case are keywords...

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 14, 2015

What would configuration for this new rule be with all these options?

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 15, 2015

My plan for options of this rule:

{
    "keyword-spacing": [2, {"before": true, "after": true, "overrides": null}]
}
  • "before" (boolean, default is true)
    • If true, this rule checks there are space(s) between a keyword and the previous token of the token. If there are not the space(s) followed by a keyword, it warns the keyword.
    • If false, this rule checks there are not space(s) between a keyword and the previous token of the token. If there are the space(s) followed by a keyword, it warns the keyword.
    • If the previous token is none of ), ], and }, this rule does nothing for this.
  • "after" (boolean, default is true)
    • If true, this rule checks there are space(s) between a keyword and the next token of the token. If there are not the space(s) preceded by a keyword, it warns the keyword.
    • If false, this rule checks there are not space(s) between a keyword and the next token of the token. If there are the space(s) preceded by a keyword, it warns the keyword.
    • If the next token is none of (, [, {, !, ~, +, -, ++, and --, this rule does nothing for this.
  • "overrides" (object|null, default is null)
    • Keys are keywords, and values are an option object to overwrite.
      For Example {"if": {"before": true, "after": false}, "case": {"after": true}}
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 15, 2015

Sounds good, let's try to get this into 2.0.0.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 15, 2015

Sounds like this would also fix: #4006

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 18, 2015

I'm working on this.
There are so many test cases!

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 23, 2015

I'm writing tests to check that it doesn't conflict with space-infix-ops.

space-infix-ops seems to not check in and instanceof operators.
Because it's checking only punctuators: https://github.com/eslint/eslint/blob/master/lib/rules/space-infix-ops.js#L33
But this operators list includes in and instanceof : https://github.com/eslint/eslint/blob/master/lib/rules/space-infix-ops.js#L14

Which is intentional behavior of space-infix-ops?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 23, 2015

@michaelficarra Do you remember what was intended for space-infix-ops? I would guess in and instanceof should be checked.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 23, 2015

@ilyavolodin Speaking as an end user, I would expect any infix operators (punctuation or word) to be checked. That said, how common is that anyway?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 23, 2015

Related discussion: #3587

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 26, 2015

Breaking: merges keyword spacing rules (fixes eslint#3869)
Also refs eslint#1338, fixes eslint#3878, fixes eslint#4006, fixes eslint#4585.

This commit creates a new rule: `keyword-spacing` (merged from
`space-after-keywords`, `space-before-keywords`, and
`space-return-throw-case`).

- `keyword-spacing` rule ignores usage of spacing at some places to not
conflict other spacing rules.
- `keyword-spacing` rule has `"overrides"` option to configure more
preference.

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 27, 2015

Breaking: merges keyword spacing rules (fixes eslint#3869)
Also refs eslint#1338, fixes eslint#3878, fixes eslint#4006, fixes eslint#4585.

This commit creates a new rule: `keyword-spacing` (merged from
`space-after-keywords`, `space-before-keywords`, and
`space-return-throw-case`).

- `keyword-spacing` rule ignores usage of spacing at some places to not
conflict other spacing rules.
- `keyword-spacing` rule has `"overrides"` option to configure more
preference.

mysticatea added a commit to mysticatea/eslint that referenced this issue Jan 5, 2016

Breaking: merges keyword spacing rules (fixes eslint#3869)
Also refs eslint#1338, fixes eslint#3878, fixes eslint#4006, fixes eslint#4585.

This commit creates a new rule: `keyword-spacing` (merged from
`space-after-keywords`, `space-before-keywords`, and
`space-return-throw-case`).

- `keyword-spacing` rule ignores usage of spacing at some places to not
conflict other spacing rules.
- `keyword-spacing` rule has `"overrides"` option to configure more
preference.

@nzakas nzakas closed this in #4811 Jan 5, 2016

@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.