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: increase lint coverage #7647

Closed
wants to merge 3 commits into from
Closed

tools: increase lint coverage #7647

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 10, 2016

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 Trott added the tools Issues and PRs related to the tools directory. label Jul 10, 2016
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 10, 2016
@targos
Copy link
Member

targos commented Jul 11, 2016

LGTM if CI is happy

@bnoordhuis
Copy link
Member

LGTM

@Trott
Copy link
Member Author

Trott commented Jul 11, 2016

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

@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

Only failure in CI is a FreeBSD build failure.

ChALkeR
ChALkeR reviewed Jul 12, 2016
View reviewed changes
tools/license2rtf.js
Stream = require('stream'),
inherits = require('util').inherits;
Stream = require('stream'),
inherits = require('util').inherits;
Copy link
Member

@ChALkeR ChALkeR Jul 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@silverwind silverwind Jul 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ChALkeR ChALkeR Jul 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@Trott
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.
@Trott
squash: nits
f44e5eb
@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

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

silverwind
silverwind reviewed Jul 12, 2016
View reviewed changes
.eslintignore
test/disabled
test/tmp*/
tools/doc/node_modules
tools/eslint
**/node_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be reduced to node_modules

Copy link
Member Author

Choose a reason for hiding this comment

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

@silverwind Sure seems like it. Done!

@silverwind
Copy link
Contributor

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

@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

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
Copy link
Member

ChALkeR commented Jul 12, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

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
@Trott
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
Copy link
Member Author

Trott commented Jul 13, 2016

Landed in cbbddc4

@Trott Trott closed this Jul 13, 2016
evanlucas pushed a commit that referenced this pull request Jul 15, 2016
@Trott @evanlucas
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 evanlucas mentioned this pull request Jul 18, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
@Trott @evanlucas
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
Copy link
Contributor

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

@Trott Trott mentioned this pull request Aug 31, 2016
Closed
@Trott
Copy link
Member Author

Trott commented Aug 31, 2016

@thealphanerd #8349

MylesBorins pushed a commit that referenced this pull request Sep 7, 2016
@Trott
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
@Trott
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
@Trott @rvagg
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
@Trott @MylesBorins
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 MylesBorins mentioned this pull request Oct 26, 2016
Closed
@Trott Trott deleted the tools branch January 13, 2022 22:43
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
@Trott @targos @bnoordhuis @silverwind @ChALkeR @MylesBorins @nodejs-github-bot