FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: increase lint coverage by Trott · Pull Request #7647 · nodejs/node · GitHub

/ node Public

tools: increase lint coverage - #7647

Closed
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:tools
ConversationCommits (3)ChecksFiles changed
Closed

tools: increase lint coverage#7647
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:tools

Conversation

Trott commented Jul 10, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

Description of change

Extend linting to tools/license2rtf.js and any other JS that gets added
to the tools directory by default.

This incidentally simplifies lint invocation and .eslintignore file.

Trott added the tools Issues and PRs related to the tools directory. label Jul 10, 2016
nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 10, 2016

targos commented Jul 11, 2016

Copy link
Copy Markdown
Member

LGTM if CI is happy

bnoordhuis commented Jul 11, 2016

Copy link
Copy Markdown
Member

LGTM

Trott commented Jul 11, 2016

Copy link
Copy Markdown
Member Author

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

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

Only failure in CI is a FreeBSD build failure.

ChALkeR reviewed Jul 12, 2016
View reviewed changes
Comment thread tools/license2rtf.js Outdated

ChALkeR Jul 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not sure. Perhaps this should use separate declarations and const instead, if it's already being changed?
i.e.

const assert = require('assert');
const Stream = require('stream');
const inherits = require('util').inherits;

The same for other variable declarations changed by this commit. Thoughts?

silverwind Jul 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree. We changed a ton of these multiline declarations a while ago, so it would be more consistent to also have seperate declarations here.

ChALkeR Jul 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Btw, eslint has a rule that could be used to forbid multiline declarations: one-var. Perhaps we should that on that sometime, if multiline declarations are already cleaned up in most places?

silverwind Jul 12, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

one-var: [2, {uninitialized: never}] gives 26 errors, 4 in lib which looks acceptable. Also doing it for initialized variables gives over 100 though.

Trott Jul 12, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@ChALkeR @silverwind nits addressed, rebased against master, force pushed, PTAL

Trott added 2 commits July 12, 2016 14:58
tools: increase lint coverage
ff4e01e 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.
squash: nits
f44e5eb

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

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

silverwind reviewed Jul 12, 2016
View reviewed changes
Comment thread .eslintignore Outdated
test/tmp*/
tools/doc/node_modules
tools/eslint
**/node_modules

silverwind Jul 12, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this can be reduced to node_modules

Trott Jul 12, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@silverwind Sure seems like it. Done!

silverwind commented Jul 12, 2016

Copy link
Copy Markdown
Contributor

LGTM, might wanna restart CI to make sure the last change works.

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

might wanna restart CI to make sure the last change works.

Seems like overkill, but Overkill is my middle name, so CI: https://ci.nodejs.org/job/node-test-pull-request/3270/

ChALkeR commented Jul 12, 2016

Copy link
Copy Markdown
Member

LGTM

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

Two build failures, but no test failures on CI. Running again: https://ci.nodejs.org/job/node-test-pull-request/3272/

Trott added a commit to Trott/io.js that referenced this pull request Jul 13, 2016
tools: increase lint coverage
cbbddc4 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

PR-URL: nodejs#7647
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Trott commented Jul 13, 2016

Copy link
Copy Markdown
Member Author

Landed in cbbddc4

Trott closed this Jul 13, 2016
evanlucas pushed a commit that referenced this pull request Jul 15, 2016
tools: increase lint coverage
9db861b 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

PR-URL: #7647
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
evanlucas mentioned this pull request Jul 18, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
tools: increase lint coverage
2a84da5 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

PR-URL: #7647
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

MylesBorins commented Aug 30, 2016

Copy link
Copy Markdown
Contributor

@Trott this is not landing cleanly, would you be willing to bacakport?

Trott mentioned this pull request Aug 31, 2016
Closed

Trott commented Aug 31, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd #8349

MylesBorins pushed a commit that referenced this pull request Sep 7, 2016
tools: increase lint coverage
6123520 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

Ref: #8349
PR-URL: #7647
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
tools: increase lint coverage
1ee0404 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

Ref: #8349
PR-URL: #7647
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
tools: increase lint coverage
cd12170 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

Ref: #8349
PR-URL: #7647
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
tools: increase lint coverage
3f4a5fe 
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

Ref: #8349
PR-URL: #7647
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins mentioned this pull request Oct 26, 2016
Closed
Trott deleted the tools branch January 13, 2022 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. 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.

7 participants

Back | FazBrowse Home | New Git URL