tools: apply stricter indentation rules to tools #13758

Trott · 2017-06-18T05:40:40Z

ESLint 4.0.0 provides stricter (and more granular) indentation checking
than previous versions. Apply the stricter indentation rules to the
tools directory.

In preparation for applying the more strict indentation linting
available in ESLint 4.0.0, correct minor indentation issues in
tools/eslint-rules/required-modules.js.

This is the only file with indentation that does not conform to the
stricter checks.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools

not-an-aardvark · 2017-06-18T06:12:13Z

tools/eslint-rules/required-modules.js

Unrelated to this PR: Should this line be return foundModules.indexOf(module) === -1; instead?

or even requiredModules.filter(module => !foundModules.includes(module))

Ack, fixed.

gibfahn · 2017-06-18T12:26:40Z

Oh cool, I wonder how this applies to test/...

./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js test ✖ 2460 problems (2460 errors, 0 warnings) 2460 errors, 0 warnings potentially fixable with the `--fix` option.

Okay, wonder how many can be --fix ed

./node tools/eslint/bin/eslint.js --cache --fix --rulesdir=tools/eslint-rules --ext=.js test ✖ 1923 problems (1923 errors, 0 warnings) 1917 errors, 0 warnings potentially fixable with the `--fix` option.

@not-an-aardvark do you know why things like this can't be --fixed? Is it just a question of someone implementing it, or is is more complex (i.e. the surrounding lines would have to be refactored)?

/Users/gib/wrk/com/node/test/sequential/test-pipe.js104:1errorExpectedindentationof6spacesbutfound4indent105:1errorExpectedindentationof6spacesbutfound4indent106:9errorExpectedindentationof6spacesbutfound8indent-legacy107:9errorExpectedindentationof6spacesbutfound8indent-legacy108:7errorExpectedindentationof4spacesbutfound6indent-legacy109:1errorExpectedindentationof4spacesbutfound2indent

targos · 2017-06-18T12:33:02Z

I guess you need to disable indent-legacy.

gibfahn · 2017-06-18T14:02:08Z

Looking at the docs, I thought indent was a superset of indent-legacy, so I assumed that wouldn't be an issue, but I could be wrong.

not-an-aardvark · 2017-06-18T19:56:32Z

It is mostly a superset, but I think there are a few cases where indent-legacy has a bug and tries to enforce the wrong indentation.

In preparation for applying the more strict indentation linting available in ESLint 4.0.0, correct minor indentation issues in tools/eslint-rules/required-modules.js. This is the only file with indentation that does not conform to the stricter checks.

ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory.

Fix previously-unnoticed typo in `required-modules.js`. Refs: nodejs#13758 (comment)

Trott · 2017-06-19T18:18:11Z

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

mhdawson

LGTM

In preparation for applying the more strict indentation linting available in ESLint 4.0.0, correct minor indentation issues in tools/eslint-rules/required-modules.js. This is the only file with indentation that does not conform to the stricter checks. PR-URL: nodejs#13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory. PR-URL: nodejs#13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Fix previously-unnoticed typo in `required-modules.js`. Refs: nodejs#13758 (comment) PR-URL: nodejs#13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Trott · 2017-06-23T03:35:53Z

Landed in 491f838, ea36609, and 25b4d87

In preparation for applying the more strict indentation linting available in ESLint 4.0.0, correct minor indentation issues in tools/eslint-rules/required-modules.js. This is the only file with indentation that does not conform to the stricter checks. PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory. PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Fix previously-unnoticed typo in `required-modules.js`. Refs: #13758 (comment) PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins · 2017-07-17T18:45:45Z

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

Trott · 2017-07-18T06:41:00Z

@MylesBorins Some required stuff probably landed in the last 12 hours, as all three commits here cherry-pick cleanly for me to v6.x-staging now. Can you try again?

MylesBorins · 2017-07-18T18:15:09Z

@Trott getting failures when landing this

/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release -J \ doctool inspector known_issues message pseudo-tty parallel sequential addons [02:04|% 100|+ 1357|- 0]: Done /Applications/Xcode.app/Contents/Developer/usr/bin/make lint Running JS linter... ./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \ benchmark lib test tools /Users/mborins/code/node/v6.x/tools/.eslintrc.yaml: Configuration for rule "indent" is invalid: Value "off" is the wrong type. Error: /Users/mborins/code/node/v6.x/tools/.eslintrc.yaml: Configuration for rule "indent" is invalid: Value "off" is the wrong type. at validateRuleOptions (/Users/mborins/code/node/v6.x/tools/eslint/lib/config/config-validator.js:109:15) at Object.keys.forEach.id (/Users/mborins/code/node/v6.x/tools/eslint/lib/config/config-validator.js:156:13) at Array.forEach (native) at Object.validate (/Users/mborins/code/node/v6.x/tools/eslint/lib/config/config-validator.js:155:35) at Object.load (/Users/mborins/code/node/v6.x/tools/eslint/lib/config/config-file.js:559:19) at loadConfig (/Users/mborins/code/node/v6.x/tools/eslint/lib/config.js:63:33) at getLocalConfig (/Users/mborins/code/node/v6.x/tools/eslint/lib/config.js:130:29) at Config.getConfig (/Users/mborins/code/node/v6.x/tools/eslint/lib/config.js:260:26) at hashOfConfigFor (/Users/mborins/code/node/v6.x/tools/eslint/lib/cli-engine.js:599:41) at executeOnFile (/Users/mborins/code/node/v6.x/tools/eslint/lib/cli-engine.js:648:32)

In preparation for applying the more strict indentation linting available in ESLint 4.0.0, correct minor indentation issues in tools/eslint-rules/required-modules.js. This is the only file with indentation that does not conform to the stricter checks. PR-URL: nodejs#13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory. PR-URL: nodejs#13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Fix previously-unnoticed typo in `required-modules.js`. Refs: nodejs#13758 (comment) PR-URL: nodejs#13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Trott · 2017-07-19T04:54:20Z

@MylesBorins Backport in #14360

In preparation for applying the more strict indentation linting available in ESLint 4.0.0, correct minor indentation issues in tools/eslint-rules/required-modules.js. This is the only file with indentation that does not conform to the stricter checks. Backport-PR-URL: #14360 PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory. Backport-PR-URL: #14360 PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Fix previously-unnoticed typo in `required-modules.js`. Backport-PR-URL: #14360 Refs: #13758 (comment) PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

In preparation for applying the more strict indentation linting available in ESLint 4.0.0, correct minor indentation issues in tools/eslint-rules/required-modules.js. This is the only file with indentation that does not conform to the stricter checks. Backport-PR-URL: #14360 PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory. Backport-PR-URL: #14360 PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Fix previously-unnoticed typo in `required-modules.js`. Backport-PR-URL: #14360 Refs: #13758 (comment) PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

In preparation for applying the more strict indentation linting available in ESLint 4.0.0, correct minor indentation issues in tools/eslint-rules/required-modules.js. This is the only file with indentation that does not conform to the stricter checks. Backport-PR-URL: #14360 PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory. Backport-PR-URL: #14360 PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Fix previously-unnoticed typo in `required-modules.js`. Backport-PR-URL: #14360 Refs: #13758 (comment) PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Trott added the tools Issues and PRs related to the tools directory. label Jun 18, 2017

not-an-aardvark reviewed Jun 18, 2017
View reviewed changes

not-an-aardvark approved these changes Jun 18, 2017
View reviewed changes

vsemozhetbyt approved these changes Jun 18, 2017
View reviewed changes

cjihrig approved these changes Jun 18, 2017
View reviewed changes

jasnell approved these changes Jun 19, 2017
View reviewed changes

Trott added 3 commits June 19, 2017 11:11

tools: apply stricter indentation rules to tools
9a2a30c
ESLint 4.0.0 provides stricter (and more granular) indentation checking than previous versions. Apply the stricter indentation rules to the tools directory.

tools: fix error in custom ESLint rule
8b834c3
Fix previously-unnoticed typo in `required-modules.js`. Refs: nodejs#13758 (comment)

Trott force-pushed the tools-indenting branch from dd13428 to 8b834c3Compare June 19, 2017 18:14

mhdawson approved these changes Jun 20, 2017
View reviewed changes

Trott closed this Jun 23, 2017

addaleax mentioned this pull request Jun 24, 2017
v8.1.3 proposal #13861
Merged

addaleax mentioned this pull request Jun 24, 2017
v8.2.0 proposal #13744
Merged

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

Trott mentioned this pull request Jul 19, 2017
[v6.x backport] tools: apply stricter indentation rules to tools #14360
Closed

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Jul 21, 2017

MylesBorins mentioned this pull request Jul 21, 2017
v6.11.2 proposal #14356
Merged

Trott deleted the tools-indenting branch January 13, 2022 22:45

Uh oh!

tools: apply stricter indentation rules to tools #13758

tools: apply stricter indentation rules to tools #13758

Uh oh!

Conversation

Trott commented Jun 18, 2017

Checklist

Affected core subsystem(s)

Uh oh!

not-an-aardvarkJun 18, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGuJun 18, 2017

Choose a reason for hiding this comment

Uh oh!

TrottJun 19, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

targos commented Jun 18, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

not-an-aardvark commented Jun 18, 2017

Uh oh!

Trott commented Jun 19, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 23, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

Trott commented Jul 18, 2017

Uh oh!

MylesBorins commented Jul 18, 2017

Uh oh!

Trott commented Jul 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants