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

@@ -77,13 +77,13 @@ module.exports = function(context) {
          function(module) {
            return foundModules.indexOf(module === -1);


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.js
  104:1  error  Expected indentation of 6 spaces but found 4  indent
  105:1  error  Expected indentation of 6 spaces but found 4  indent
  106:9  error  Expected indentation of 6 spaces but found 8  indent-legacy
  107:9  error  Expected indentation of 6 spaces but found 8  indent-legacy
  108:7  error  Expected indentation of 4 spaces but found 6  indent-legacy
  109:1  error  Expected indentation of 4 spaces but found 2  indent

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 8b834c3 Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: apply stricter indentation rules to tools #13758

tools: apply stricter indentation rules to tools #13758

Trott commented Jun 18, 2017

not-an-aardvark Jun 18, 2017

TimothyGu Jun 18, 2017

Trott Jun 19, 2017

gibfahn commented Jun 18, 2017

targos commented Jun 18, 2017

gibfahn commented Jun 18, 2017

not-an-aardvark commented Jun 18, 2017

Trott commented Jun 19, 2017

mhdawson left a comment

Trott commented Jun 23, 2017

MylesBorins commented Jul 17, 2017

Trott commented Jul 18, 2017

MylesBorins commented Jul 18, 2017

Trott commented Jul 19, 2017

		@@ -77,13 +77,13 @@ module.exports = function(context) {
		function(module) {
		return foundModules.indexOf(module === -1);

tools: apply stricter indentation rules to tools #13758

tools: apply stricter indentation rules to tools #13758

Conversation

Trott commented Jun 18, 2017

Checklist

Affected core subsystem(s)

not-an-aardvark Jun 18, 2017

Choose a reason for hiding this comment

TimothyGu Jun 18, 2017

Choose a reason for hiding this comment

Trott Jun 19, 2017

Choose a reason for hiding this comment

gibfahn commented Jun 18, 2017

targos commented Jun 18, 2017

gibfahn commented Jun 18, 2017

not-an-aardvark commented Jun 18, 2017

Trott commented Jun 19, 2017

mhdawson left a comment

Choose a reason for hiding this comment

Trott commented Jun 23, 2017

MylesBorins commented Jul 17, 2017

Trott commented Jul 18, 2017

MylesBorins commented Jul 18, 2017

Trott commented Jul 19, 2017