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

Enforce the amount of whitespace surrounding keywords. #7621

Closed
Wilfred opened this issue Nov 18, 2016 · 8 comments
Closed

Enforce the amount of whitespace surrounding keywords. #7621

Wilfred opened this issue Nov 18, 2016 · 8 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 bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@Wilfred
Copy link
Contributor

Wilfred commented Nov 18, 2016

Given the following tab-indented source code:

function foo() {
	if (true) {
		try {
			bar();
		}
		catch (err)
		{
			baz();
		}
	}
}

Using brace-style to use "1tbs", we end up with tabs around the catch keyword.

function foo() {
	if (true) {
		try {
			bar();
		}		catch (err)		{
			baz();
		}
	}
}

It doesn't seem possible to fix this with the existing eslint rules:

  • keyword-spacing only asserts that there's a non-zero amount of whitespace around keywords
  • no-multi-spaces ignore tabs
  • no-tabs isn't useful because we're using tabs for indentation too

Possible solutions:

  • add an option to keyword-spacing to enforce exactly one space (my suggestion)
  • add an option to no-tabs to only flag tabs that aren't at the beginning of the line (suggested by @platinumazure)
  • generalise no-multi-spaces to work for arbitrary whitespace, not just spaces

Tested on eslint 3.10.2.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Nov 18, 2016
@not-an-aardvark not-an-aardvark added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Nov 18, 2016
@platinumazure
Copy link
Member

One of the reasons I favor augmenting no-tabs is because this could also supplement other space-enforcing rules, not just keyword-spacing:

if (foo) {    // Tab between condition close paren and open brace
}

The above example wouldn't be enforced by keyword-spacing because there isn't a keyword on either side of the hypothetical tab. So this implies that space-before-blocks and similar rules would also need to be enforced.

On the other hand, no-tabs is already about flagging tabs, so it would just be a matter of adding an option to allow tabs at the beginning of a line for indentation and flag all other tabs. That would take care of all of the possible use cases, I think.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Nov 18, 2016

I would prefer the option of generalizing no-multi-spaces to flag multiple tabs.

I don't think adding a "prohibit multiple whitespace characters" option to keyword-spacing is a good idea, because (a) it would directly conflict with no-multi-spaces, and (b) we would have to add a similar option to a lot of other spacing rules.

Adding an option to no-tabs is better in that we would only have to modify one rule, but I think it would be overloading the intended purpose of the no-tabs rule, which is to disallow all tabs. On the other hand, the no-multi-spaces rule is explicitly intended to handle consecutive whitespace in cases like this. It shouldn't matter whether that whitespace happens to be spaces or tabs.

To expand on that, no-multi-spaces already handles cases like this:

try {
}             catch (err) { // same as above, but with spaces
}

I think it would be unexpected to handle that case with a different rule depending on whether the gap contains spaces or tabs.

@platinumazure
Copy link
Member

@not-an-aardvark @Wilfred What should happen in the case where two tokens are separated by one tab?

@Wilfred Would you want that case flagged?

@not-an-aardvark If we wanted to flag that particular case, which rule should be responsible?

@not-an-aardvark
Copy link
Member

What should happen in the case where two tokens are separated by one tab?

I think the rule should not report an error.

An alternative would be to update rules such as keyword-spacing to actually mandate space characters, rather than generic whitespace.

If we wanted to flag that particular case, which rule should be responsible?

I'm not sure I understand what you mean by "that particular case". If you're referring to the code sample in my comment above, I think no-multi-spaces should handle it. (My preference would be for no-multi-spaces to report consecutive tabs as well as consecutive spaces.)

@platinumazure
Copy link
Member

@not-an-aardvark Sorry, "that particular case" was "the case where two tokens are separated by one tab". If you believe that shouldn't be an error at all, then the question is moot.

not-an-aardvark added a commit that referenced this issue Apr 22, 2017
Previously, the brace-style autofixer would leave any existing whitesapce between tokens when removing newlines. This would result in a large amount of extra whitespace on a line when fixing indented code. Users generally don't expect indentation whitespace to be preserved inline when fixing brace styling, so this commit updates the fixer to always output a single space between tokens when removing newlines.
@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly and removed enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 22, 2017
@bardware
Copy link

bardware commented May 4, 2017

Hi,

in what version will these changes take effect? I pulled npm install eslint@4.0.0-alpha.1 -g with the result being:

			}
			else {

still becomes

			}			else {

@platinumazure
Copy link
Member

Hi @bardware, the changes should be in this week's release.

@bardware
Copy link

bardware commented May 8, 2017

Seen it working in Alpha.2. Very nice, thanks alot!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 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 6, 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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

5 participants