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

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.

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

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.

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.

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

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

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

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

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

@Fishrock123 will do

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

Still present in rc-3.

eslint/eslint#3164

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?

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.

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.

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

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

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?

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.

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

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

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

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

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

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.

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

Thanks! Landed in 162e16a and 0ec093c.

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

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