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.

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

tools: avoid using process.cwd in tools/lint-js #17121

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@tniessen
Member

tniessen commented Nov 18, 2017

The first occurrence of path.join() can be replaced with path.resolve(). The second occurrence should be unnecessary as ESLint will resolve the path internally, and the old check probably did not work as intended anyway.

CI: https://ci.nodejs.org/job/node-test-linter/13675/

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

tools

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

@tniessen tniessen requested a review from mscdex Nov 18, 2017

@lpinca

lpinca approved these changes Nov 19, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 19, 2017

Member

@tniessen Would you mind kicking off CI when you add that label? :)

CI: https://ci.nodejs.org/job/node-test-commit/14149/

Member

addaleax commented Nov 19, 2017

@tniessen Would you mind kicking off CI when you add that label? :)

CI: https://ci.nodejs.org/job/node-test-commit/14149/

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Nov 19, 2017

Member

@addaleax I kicked off CI before creating the PR, the link is in the description. Unless, of course, node-test-linter is not enough (I assumed it is, but I could be wrong).

Member

tniessen commented Nov 19, 2017

@addaleax I kicked off CI before creating the PR, the link is in the description. Unless, of course, node-test-linter is not enough (I assumed it is, but I could be wrong).

@tniessen tniessen referenced this pull request Nov 19, 2017

Closed

tools: use built-in padStart instead of padString #17120

2 of 2 tasks complete
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 19, 2017

Member

@tniessen Ah – Yeah, I didn’t see that because it didn’t show up in the github display here, but that makes sense if you used node-test-commit directly 👍

Member

addaleax commented Nov 19, 2017

@tniessen Ah – Yeah, I didn’t see that because it didn’t show up in the github display here, but that makes sense if you used node-test-commit directly 👍

@refack

refack approved these changes Nov 20, 2017

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Nov 21, 2017

Member

Landed in f82f5a4.

Member

tniessen commented Nov 21, 2017

Landed in f82f5a4.

@tniessen tniessen closed this Nov 21, 2017

tniessen added a commit that referenced this pull request Nov 21, 2017

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

PR-URL: #17121
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@addaleax addaleax removed the author ready label Nov 28, 2017

MylesBorins added a commit that referenced this pull request Dec 12, 2017

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

PR-URL: #17121
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 12, 2017

Merged

v9.3.0 proposal #17631

MylesBorins added a commit that referenced this pull request Dec 19, 2017

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

PR-URL: #17121
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn added a commit that referenced this pull request Dec 19, 2017

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

PR-URL: #17121
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn added a commit that referenced this pull request Dec 19, 2017

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

PR-URL: #17121
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Closed

v8.9.4 proposal #17772

gibfahn added a commit that referenced this pull request Dec 20, 2017

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

PR-URL: #17121
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Merged

v8.9.4 proposal #17774

@MylesBorins MylesBorins referenced this pull request Dec 20, 2017

Merged

v6.12.3 proposal #17776

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

tools: avoid using process.cwd in tools/lint-js
The first occurrence of path.join() can easily be replaced with
path.resolve().
The second occurrence should be unnecessary as ESLint will resolve the
path internally, and the old check probably did not work as intended
anyway.

PR-URL: nodejs#17121
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment