Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools: enable stricter linting in lib directory #14403

Closed
wants to merge 2 commits into from
Closed

tools: enable stricter linting in lib directory #14403

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 20, 2017

First commit:

lib: adjust indentation for impending lint change

ESLint 4.x provides stricter indentation linting than previous versions.
In preparation for enabling the stricter indentation linting, adjust the
indentation of four lines in lib/net.js and lib/repl.js.

Second commit:

tools: enable stricter linting in lib directory

Enable ESLint 4.x linting and disable legacy linting in the lib
directory.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools lib net repl

@nodejs-github-bot nodejs-github-bot added net Issues and PRs related to the net subsystem. repl Issues and PRs related to the REPL subsystem. labels Jul 20, 2017
@Trott Trott added tools Issues and PRs related to the tools directory. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 20, 2017
@Trott
Copy link
Member Author

Trott commented Jul 20, 2017

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

evanlucas
evanlucas reviewed Jul 20, 2017
View reviewed changes
lib/repl.js Outdated
@@ -1058,7 +1058,7 @@ REPLServer.prototype.defineCommand = function(keyword, cmd) {
cmd = {action: cmd};
} else if (typeof cmd.action !== 'function') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'action', 'function', cmd.action);
'action', 'function', cmd.action);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not break on all arguments if we break on any? For some reason I thought that was the more used style throughout the codebase

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not noticed that as the predominant style, although I would welcome that change personally. I'll do that here, if no one objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, doesn't make much of a difference to me, so up to you. I was mainly just curious. Maybe that was just for cpp code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Too late, I did it. :-D

mscdex
mscdex reviewed Jul 20, 2017
View reviewed changes
lib/.eslintrc.yaml Outdated
@@ -1,4 +1,13 @@
rules:
indent: [error, 2, {ArrayExpression: first,
CallExpression: {arguments: first},
Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of indentation, shouldn't these be indented a bit further to align with the third argument above it? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Yes they should. 😐

@Trott
lib: adjust indentation for impending lint change
c3b8f5a 
ESLint 4.x provides stricter indentation linting than previous versions.
In preparation for enabling the stricter indentation linting, adjust the
indentation of four lines in lib/net.js and lib/repl.js.
@Trott
tools: enable stricter linting in lib directory
43fc451 
Enable ESLint 4.x linting and disable legacy linting in the lib
directory.

While doing this, some indentation in the .eslintrc.yaml file itself was
noticed to be incorrect. Fixed that too.
@Trott
Copy link
Member Author

Trott commented Jul 23, 2017

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

@refack
Copy link
Contributor

refack commented Jul 23, 2017 •
edited
Loading

Just for my info - why just in /lib/? found the others

This was referenced Jul 24, 2017
Closed
Closed
@Trott
Copy link
Member Author

Trott commented Jul 24, 2017

Landed in 8cccdd9 and 7f5a745

@Trott Trott closed this Jul 24, 2017
Trott added a commit that referenced this pull request Jul 24, 2017
@Trott
lib: adjust indentation for impending lint change
8cccdd9 
ESLint 4.x provides stricter indentation linting than previous versions.
In preparation for enabling the stricter indentation linting, adjust the
indentation of four lines in lib/net.js and lib/repl.js.

PR-URL: #14403
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Trott added a commit that referenced this pull request Jul 24, 2017
@Trott
tools: enable stricter linting in lib directory
7f5a745 
Enable ESLint 4.x linting and disable legacy linting in the lib
directory.

While doing this, some indentation in the .eslintrc.yaml file itself was
noticed to be incorrect. Fixed that too.

PR-URL: #14403
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
@addaleax
Copy link
Member

Marking as dont-land-on-v8.x because it breaks linting but feel free to backport.

Trott added a commit to Trott/io.js that referenced this pull request Jul 27, 2017
@Trott
lib: adjust indentation for impending lint change
5b05f07 
ESLint 4.x provides stricter indentation linting than previous versions.
In preparation for enabling the stricter indentation linting, adjust the
indentation of four lines in lib/net.js and lib/repl.js.

PR-URL: nodejs#14403
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Jul 27, 2017
@Trott
tools: enable stricter linting in lib directory
de37588 
Enable ESLint 4.x linting and disable legacy linting in the lib
directory.

While doing this, some indentation in the .eslintrc.yaml file itself was
noticed to be incorrect. Fixed that too.

PR-URL: nodejs#14403
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
@Trott Trott mentioned this pull request Jul 27, 2017
Closed
2 tasks
@Trott
Copy link
Member Author

Trott commented Jul 27, 2017

@addaleax Backported in #14520.

addaleax pushed a commit that referenced this pull request Jul 27, 2017
@Trott @addaleax
lib: adjust indentation for impending lint change
9e5a088 
ESLint 4.x provides stricter indentation linting than previous versions.
In preparation for enabling the stricter indentation linting, adjust the
indentation of four lines in lib/net.js and lib/repl.js.

Backport-PR-URL: #14520
Backport-Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14403
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 27, 2017
@Trott @addaleax
tools: enable stricter linting in lib directory
d11840c 
Enable ESLint 4.x linting and disable legacy linting in the lib
directory.

While doing this, some indentation in the .eslintrc.yaml file itself was
noticed to be incorrect. Fixed that too.

Backport-PR-URL: #14520
Backport-Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14403
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
@Trott Trott deleted the lib-indent branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@aqrln aqrln aqrln approved these changes

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@evanlucas evanlucas evanlucas approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. net Issues and PRs related to the net subsystem. repl Issues and PRs related to the REPL subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.