tools: eslint object curly spacing by sebdeckers · Pull Request #14162 · nodejs/node

sebdeckers · 2017-07-11T01:23:43Z

As per suggestion by @jasnell, this is a de-facto house style. Enabling this rule makes it de-jure.

Not sure how the commit message guideline would make me label this commit, so I've left it as tools: even though the (hopefully inert) lint fixes touch other areas.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes (@sebdeckers: works on my machine 😅)
commit message follows commit guidelines (as per comment below)

Affected core subsystem(s)

tools

refack · 2017-07-11T01:36:25Z

Not sure how the commit message guideline would make me label this commit, so I've left it as tools: even though the (hopefully inert) lint fixes touch other areas.

IMHO tools: is the right prefix

mscdex · 2017-07-11T01:37:29Z

We should leave out changes to upstream packages (e.g. files in tools/icu).

refack · 2017-07-11T01:37:00Z

.eslintrc.yaml

No need for " in yaml

I need to take another look

refack · 2017-07-11T01:40:03Z

/cc @nodejs/release

sebdeckers · 2017-07-11T01:57:15Z

Thanks for the super quick feedback! ❤️
@mscdex I've undone the change and added tools/icu to .eslintignore
@refack cleaned up the quotes

Q: Should I squash the commits, either while pushing changes in response to reviews, or once the changes have been approved?

refack · 2017-07-11T01:59:10Z

Q: Should I squash the commits, either while pushing changes in response to reviews, or once the changes have been approved?

Just adding commits is the best way for us to review. Commits will probably be squashed while landing.
Conflicts might arise before this PR lands, in that case you'll probably need to rebase and fix the conflicts.

Trott · 2017-07-11T03:11:43Z

.eslintrc.yaml

You can reduce the churn slightly and conform more closely to the house style by doing this instead:

object-curly-spacing: [error, always, {objectsInObjects: false}]

That flags 2144 problems in the current code base, instead of the 2183 flagged with the PR as it is. That's not a huge reduction, but every little bit helps to make backporting less awful.

99% of these changes were made using eslint --fix automatically. Except for ~30 cases where the insertion of spaces then caused the line to grow longer than 80 characters, which I cleaned up manually following my own stylistic judgement and ensuring eslint passes.

In general it's useful to do the automatic ones as one commit, and then the manual ones as a separate one. They'll get squashed on landing, but it means people can review the ones that weren't just an eslint --fix (which are probably the more interesting ones).

mscdex · 2017-07-11T05:04:46Z

Perhaps we should also add 'deps/' to .eslintignore (assuming that will match everything in deps).

Trott · 2017-07-11T05:40:06Z

Perhaps we should also add 'deps/' to .eslintignore (assuming that will match everything in deps).

We don't run ESLint on the deps directory so there's no need to ignore files in it. We explicitly run it on benchmark, doc, lib, tools, and test directories only.

vsemozhetbyt · 2017-07-11T10:25:18Z

If the rule remains as it is now in the main .eslintrc.yaml, you can delete it from the doc/.eslintrc.yaml.

sebdeckers · 2017-07-11T11:00:36Z

@vsemozhetbyt Done. Thank you for the suggestion!

vsemozhetbyt

LGTM if full CI is green when unblocked (in case I've missed something).

refack · 2017-07-11T12:20:49Z

I'm thinking the main commit should be split to 5 (since each directory have different backporting concern):

/benchmark/
/lib/
/test/
/tools/
.eslintrc.yamls

Although this should be a 0 semantic change, and could be backported as a whole.
But @nodejs/release should give the final word

Trott · 2017-07-11T16:03:13Z

I'm thinking the main commit should be split to 5

I'm not saying it's better or worse, but in the past, we've often done it as two commits: One for the code updates, and one for enabling the rule in eslintrc.

gibfahn · 2017-07-11T23:08:33Z

I'm thinking the main commit should be split to 5 (since each directory have different backporting concern):

I think as they're just using eslint --fix they'll just be backported by backporting the rule and then running eslint --fix again, so in this specific case I don't think it's necessary.

vsemozhetbyt · 2017-07-11T23:15:13Z

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

Trott · 2017-07-11T23:54:07Z

@sebdeckers The name of this PR should say object curly spacing rather than template curly spacing. Or am I mistaken?

evanlucas · 2017-07-12T19:28:08Z

I think I'm -1 on this change. I wouldn't say the house style is to include spaces, especially if this requires 292 changes...

sebdeckers · 2017-07-13T02:36:43Z

@evanlucas I didn't mean for this change to be about bikeshedding eslint rules. "House style" was quoted from the referenced comment; not my words. As for the high change count, I'd argue that proves the need for this option to be set one way or another.

TimothyGu

I'd like to standardize around array spacing too. The preferred style, I believe, is to not add spaces inside brackets.

refack · 2017-07-13T11:29:53Z

I think I'm -1 on this change. I wouldn't say the house style is to include spaces, especially if this requires 292 changes...

The other side of the argument — setting:

object-curly-spacing: [error, never]

results in 3646 errors in 517 files

jasnell

largely rubber stamp lgtm

sebdeckers · 2017-07-15T00:42:44Z

Rebased and delinted recent commits.

vsemozhetbyt · 2017-07-21T17:18:44Z

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

vsemozhetbyt · 2017-07-21T17:35:04Z

Linter fails:

test/parallel/test-fs-open-flags.js
  59:23  error  A space is required after '{'   object-curly-spacing
  59:70  error  A space is required before '}'  object-curly-spacing

test/parallel/test-internal-fs.js
  12:3   error  A space is required after '{'   object-curly-spacing
  12:59  error  A space is required before '}'  object-curly-spacing

PR-URL: nodejs#14162 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

refack · 2017-07-21T19:15:34Z

Landed in 6add5b3 bb29405

refack · 2017-07-21T19:21:37Z

Extra sanity test on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7482/
Extra lint on master: https://ci.nodejs.org/job/node-test-linter/10672/

addaleax · 2017-07-22T20:20:06Z

@sebdeckers This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR.

PR-URL: nodejs#14162 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #14162 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-10-17T04:53:35Z

Since this touches so many files it would be really great to get a backport to v6.x to avoid future merge conflicts. Please follow this guide and raise a backport PR.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 11, 2017

refack previously approved these changes Jul 11, 2017

View reviewed changes

.eslintrc.yaml Outdated

Copy link

Contributor

refack Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for " in yaml

mscdex added the tools Issues and PRs related to the tools directory. label Jul 11, 2017

Trott reviewed Jul 11, 2017

View reviewed changes

vsemozhetbyt approved these changes Jul 11, 2017

View reviewed changes

refack approved these changes Jul 11, 2017

View reviewed changes

sebdeckers changed the title ~~tools: eslint template curly spacing~~ tools: eslint object curly spacing Jul 12, 2017

refack mentioned this pull request Jul 12, 2017

test: decrease duration of test-cli-syntax #14187

Closed

2 tasks

TimothyGu approved these changes Jul 13, 2017

View reviewed changes

jasnell approved these changes Jul 14, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Jul 15, 2017

tools: some eslint rules proposals #14253

Closed

sebdeckers added 2 commits July 21, 2017 23:53

tools: ignore upstream icu package when linting

1040d72

tools: de-dupe rule from docs-specific linter

267992a

refack closed this Jul 21, 2017

addaleax added the backport-requested-v8.x label Jul 22, 2017

refack mentioned this pull request Jul 24, 2017

[v8.x] test: improve the test common documentation #14454

Closed

Trott mentioned this pull request Aug 1, 2017

refactor test/sequential/test-fs-watch.js #14534

Closed

2 tasks

jasnell mentioned this pull request Sep 20, 2017

[v8.x] Backport of 14162 #15499

Closed

3 tasks

jasnell added backported-to-v8.x and removed backport-requested-v8.x labels Sep 20, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

vsemozhetbyt mentioned this pull request Sep 24, 2017

test, fs: fix chmod mask testing on windows #12835

Closed

3 tasks

MylesBorins added the backport-requested-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Oct 17, 2017

Should issues creating churn on 100s of files require backports to land? #16249

Closed

Trott mentioned this pull request Jan 25, 2018

test: improve flaky test-listen-fd-ebadf.js #17797

Closed

2 tasks

Uh oh!

Conversation

sebdeckers commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jul 11, 2017

Uh oh!

mscdex commented Jul 11, 2017

Uh oh!

refack Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 11, 2017

Uh oh!

sebdeckers commented Jul 11, 2017

Uh oh!

refack commented Jul 11, 2017

Uh oh!

Trott Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

sebdeckers Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jul 11, 2017

Uh oh!

Trott commented Jul 11, 2017

Uh oh!

vsemozhetbyt commented Jul 11, 2017

Uh oh!

sebdeckers commented Jul 11, 2017

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 11, 2017

Uh oh!

Trott commented Jul 11, 2017

Uh oh!

gibfahn commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Jul 11, 2017

Uh oh!

Trott commented Jul 11, 2017

Uh oh!

evanlucas commented Jul 12, 2017

Uh oh!

sebdeckers commented Jul 13, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 13, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

sebdeckers commented Jul 15, 2017

Uh oh!

vsemozhetbyt commented Jul 21, 2017

Uh oh!

vsemozhetbyt commented Jul 21, 2017

Uh oh!

refack commented Jul 21, 2017

Uh oh!

refack commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jul 22, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

sebdeckers commented Jul 11, 2017 •

edited

Loading

gibfahn commented Jul 11, 2017 •

edited

Loading

refack commented Jul 21, 2017 •

edited

Loading