tools: enable no-extra-semi rule in eslint #2205

targos · 2015-07-19T13:24:23Z

This CL adds two rules:

no-extra-semi

Removed obvious extra semicolons
Had to change some loops to add an empty body

semi-spacing (removed)

This one is up for discussion because the changes I had to make feel wrong to me.
For instance in try {fs.rmdirSync(tmp(folder)); } catch (ex) {} the space isn't really improving readability. Maybe it would better if we added a space as well at the beginning of the try block, but I can't find a rule for that.
Because of that I would prefer to explicitly disable this rule in .eslintrc but it is the one that enforces spaces in code like for(let i = 0; i < x; i++) so we'd lose that check.

silverwind · 2015-07-19T15:42:29Z

Oh damn, looks like we were working on this at the same time (#2207).

silverwind · 2015-07-19T15:52:37Z

Regarding semi-spacing: I use it personally in my projects, but didn't see the necessity because the extra space looks "wrong" in a try {something; } to me. I see some value in the spacing of a for loop, maybe we should propose an option like forOnly to that rule.

Regarding bodyless loops: I see you wenn with a {} as a simple workaround to the rule bug. It's one way of handling it, but I feel the refactoring I did in the two lib cases might help make them more readable.

silverwind · 2015-07-19T16:37:27Z

Closed #2207 in favor of this. Maybe you can take a hint from the few changes I did differently.

cjihrig · 2015-07-19T17:38:55Z

try {something; } does look a little... not quite right. I'd say either turn it off or add a space at the beginning as well.

silverwind · 2015-07-19T18:24:23Z

ESLint already fixed the issue with the bodyless loops in eslint/eslint@25f14ae, which will eliminate 7 changes in this diff. I'd say let's wait until the ESLint 1.0 is out, which shouldn't be too far off, and then revisit this PR.

targos · 2015-07-19T19:39:49Z

OK then let's wait for v1.0. Good job on making them fix this issue so fast !

Fishrock123 · 2015-07-20T01:05:29Z

test/pummel/test-process-hrtime.js

@@ -9,7 +9,7 @@ assert(Array.isArray(start));

 // busy-loop for 2 seconds
 var now = Date.now();
-while (Date.now() - now < 2000);
+while (Date.now() - now < 2000) {}


This style shouldn't actually be enforced. You can do useful things inside while(); and for(;;); statements.

That's what the mentioned ESlint bugfix is about: bodyless loops :)

I reverted those changes

Fishrock123 · 2015-07-20T01:10:53Z

@targos @joaocgreis @thefourtheye

I forgot to mention in the onboarding session: We try to avoid purely style changes as much as possible. This is mostly because it makes git blame harder to understand.

Some concessions can be made for bringing the style to be more cohesive though.

silverwind · 2015-07-21T00:09:02Z

.eslintrc

@@ -46,6 +48,8 @@ rules:
  comma-spacing: 2
  ## put semi-colon
  semi: 2
+  ## disable semi-spacing
+  semi-spacing: 0


might as well remove it completely ;)

targos · 2015-07-24T15:47:22Z

I started to work on the eslint update on my fork, using the latest rc2.
We have a problem because it is reporting 589 indent errors. Apparently it doesn't like variable name alignment like we do here.
I found no mention of that in the migration guide or the docs. There are a few bug fixes for this rule, maybe we are relying on the wrong behavior :(

/cc @silverwind @yosuke-furukawa

yosuke-furukawa · 2015-07-24T16:01:15Z

No rule settings to avoid the errors?? If so, we need to suggest the settings to eslint

Fishrock123 · 2015-07-24T16:18:37Z

@targos could you either report that to them, or pull one of the maintainers in here? Thanks. :)

targos · 2015-07-24T16:23:58Z

@Fishrock123 will do

cjihrig · 2015-07-24T16:26:50Z

This issue might be relevant - eslint/eslint#3139. Looks like it was fixed yesterday.

targos · 2015-07-25T11:19:30Z

Still present in rc-3.

eslint/eslint#3164

silverwind · 2015-07-25T11:38:23Z

We could do

var x;
var y;
var z;

instead of

var x,
    y,
    z;

Its easier to handle because you don't have to think whether to put a comma or semicolon on each line, but I fear a 500+ line diff might be too much noise. Another argument against the second style is that the 4-character alignment doesn't work anymore with const as opposed to var.

Any thoughts on this?

Fishrock123 · 2015-07-25T18:07:29Z

Any thoughts on this?

That'd be so purely style changes that it wouldn't be approved. That's not even a lint fixing change really. Let's just wait until it gets fixed.

silverwind · 2015-07-27T08:24:29Z

Looks the error on 4-space alignment is intended and I think I have to agree that strictly speaking it's an violation of 2-space indent. We could either go with disabling the rule, doing my suggestion above, or do a ugly

var a,
  b,
  c;

I think we should consider my suggestion, even if the diff is huge. One {const,let,var} per variable is found in style guides like https://github.com/airbnb/javascript#13.2.

targos · 2015-07-27T08:25:00Z

I also don't want to change so many lines just to content the linter.
The style that we are using (name alignment instead of 2 spaces) is nice and should be allowed.
It appears that the change in eslint is considered a bug fix so it is unlikely that it will change for 1.0

targos · 2015-07-27T08:30:14Z

I'm -1 for the ugly fix.
If we want to make a huge diff, let's be explicit and put the var keyword.

Fishrock123 · 2015-07-27T16:02:57Z

I'm for var per line if we'd have to change the indentation to two spaces.

I'm against doing either though, the diff would probably be huge... Is it unavoidable now?

silverwind · 2015-07-27T20:36:51Z

Looking through the 592 indent errors reported by rc-3, it looks like some of these are unrelated to the var style and seem bugged (especially cases after an if condition). We might be able to reduce the number of changes to like 300-400 lines (note, this includes test), but that indentation style looks to be pretty common.

Fishrock123 · 2015-07-27T21:12:58Z

@silverwind any idea why they didn't catch them before?

I'd really prefer to have an exception rule to this tbh.

targos · 2016-01-15T08:57:09Z

Done, although that's really a purely stylistic change.

silverwind · 2016-01-15T11:22:38Z

Of course it is, but we keep getting PRs for it ;)

cjihrig · 2016-01-15T19:49:11Z

The changes themselves LGTM. This is definitely churn though :-(

silverwind · 2016-01-16T16:34:20Z

LGTM. Yeah it's a bit of churn, but these are mostly on the closing bracket of functions, so shouldn't interfere with git blame that much.

silverwind · 2016-01-16T16:35:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1279/

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

silverwind · 2016-01-16T18:54:37Z

Thanks! Landed in 162e16a and 0ec093c.

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Trott · 2016-02-16T01:03:00Z

Should this go in LTS? (My feeling is it should.)

rvagg · 2016-02-16T01:09:56Z

fine by me, kill those extra semi's with fire, adding lts-watch

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

PR-URL: nodejs#2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

thefourtheye added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jul 19, 2015

silverwind mentioned this pull request Jul 19, 2015

tools: enable no-extra-semi rule in the linter #2207

Closed

Fishrock123 reviewed Jul 20, 2015
View reviewed changes

silverwind reviewed Jul 21, 2015
View reviewed changes

targos force-pushed the no-extra-semi branch from 3d0dfce to c81b1c4 Compare July 21, 2015 06:10

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

targos force-pushed the no-extra-semi branch from c81b1c4 to 07e18ce Compare January 15, 2016 08:53

targos changed the title ~~tools: enable semicolon-related rules in eslint~~ tools: enable no-extra-semi rule in eslint Jan 15, 2016

silverwind pushed a commit that referenced this pull request Jan 16, 2016

tools: enable no-extra-semi rule in eslint

162e16a

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

silverwind pushed a commit that referenced this pull request Jan 16, 2016

lib,test: remove extra semicolons

0ec093c

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

silverwind closed this Jan 16, 2016

targos deleted the no-extra-semi branch January 16, 2016 19:36

evanlucas mentioned this pull request Jan 18, 2016

v5.5.0 Release Proposal (Stable) #4742

Merged

evanlucas pushed a commit that referenced this pull request Jan 18, 2016

tools: enable no-extra-semi rule in eslint

16ef250

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

evanlucas pushed a commit that referenced this pull request Jan 18, 2016

lib,test: remove extra semicolons

f0ee088

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

rvagg added the lts-watch-v4.x label Feb 16, 2016

Trott mentioned this pull request Feb 16, 2016

tools: update ESLint to version 2.1.0 #5214

Closed

MylesBorins pushed a commit that referenced this pull request Feb 17, 2016

tools: enable no-extra-semi rule in eslint

e408e94

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins pushed a commit that referenced this pull request Feb 17, 2016

lib,test: remove extra semicolons

f661927

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Feb 17, 2016

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

tools: enable no-extra-semi rule in eslint

d10bee8

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

lib,test: remove extra semicolons

fd26960

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins mentioned this pull request Feb 18, 2016

V4.4.0 proposal #5301

Merged

MylesBorins pushed a commit that referenced this pull request Mar 2, 2016

tools: enable no-extra-semi rule in eslint

2b52b15

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins pushed a commit that referenced this pull request Mar 2, 2016

lib,test: remove extra semicolons

61570cc

PR-URL: #2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

tools: enable no-extra-semi rule in eslint

559f350

PR-URL: nodejs#2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

lib,test: remove extra semicolons

e5ed959

PR-URL: nodejs#2205 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: enable no-extra-semi rule in eslint #2205

tools: enable no-extra-semi rule in eslint #2205

targos commented Jul 19, 2015

silverwind commented Jul 19, 2015

silverwind commented Jul 19, 2015

silverwind commented Jul 19, 2015

cjihrig commented Jul 19, 2015

silverwind commented Jul 19, 2015

targos commented Jul 19, 2015

Fishrock123 Jul 20, 2015

silverwind Jul 20, 2015

targos Jul 20, 2015

Fishrock123 commented Jul 20, 2015

silverwind Jul 21, 2015

targos Jul 21, 2015

targos commented Jul 24, 2015

yosuke-furukawa commented Jul 24, 2015

Fishrock123 commented Jul 24, 2015

targos commented Jul 24, 2015

cjihrig commented Jul 24, 2015

targos commented Jul 25, 2015

silverwind commented Jul 25, 2015

Fishrock123 commented Jul 25, 2015

silverwind commented Jul 27, 2015

targos commented Jul 27, 2015

targos commented Jul 27, 2015

Fishrock123 commented Jul 27, 2015

silverwind commented Jul 27, 2015

Fishrock123 commented Jul 27, 2015

targos commented Jan 15, 2016

silverwind commented Jan 15, 2016

cjihrig commented Jan 15, 2016

silverwind commented Jan 16, 2016

silverwind commented Jan 16, 2016

silverwind commented Jan 16, 2016

Trott commented Feb 16, 2016

rvagg commented Feb 16, 2016

tools: enable no-extra-semi rule in eslint #2205

tools: enable no-extra-semi rule in eslint #2205

Conversation

targos commented Jul 19, 2015

no-extra-semi

semi-spacing (removed)

silverwind commented Jul 19, 2015

silverwind commented Jul 19, 2015

silverwind commented Jul 19, 2015

cjihrig commented Jul 19, 2015

silverwind commented Jul 19, 2015

targos commented Jul 19, 2015

Fishrock123 Jul 20, 2015

Choose a reason for hiding this comment

silverwind Jul 20, 2015

Choose a reason for hiding this comment

targos Jul 20, 2015

Choose a reason for hiding this comment

Fishrock123 commented Jul 20, 2015

silverwind Jul 21, 2015

Choose a reason for hiding this comment

targos Jul 21, 2015

Choose a reason for hiding this comment

targos commented Jul 24, 2015

yosuke-furukawa commented Jul 24, 2015

Fishrock123 commented Jul 24, 2015

targos commented Jul 24, 2015

cjihrig commented Jul 24, 2015

targos commented Jul 25, 2015

silverwind commented Jul 25, 2015

Fishrock123 commented Jul 25, 2015

silverwind commented Jul 27, 2015

targos commented Jul 27, 2015

targos commented Jul 27, 2015

Fishrock123 commented Jul 27, 2015

silverwind commented Jul 27, 2015

Fishrock123 commented Jul 27, 2015

targos commented Jan 15, 2016

silverwind commented Jan 15, 2016

cjihrig commented Jan 15, 2016

silverwind commented Jan 16, 2016

silverwind commented Jan 16, 2016

silverwind commented Jan 16, 2016

Trott commented Feb 16, 2016

rvagg commented Feb 16, 2016