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: fix update-eslint.sh #14850

Merged
merged 1 commit into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@MylesBorins
Member

MylesBorins commented Aug 16, 2017

The script currently assumes that there is a package.json in
eslint-tmp. If there isn't the logic of the script fails.
This adds a call to npm init --yes ensuring there is a package.json
and that the script can do it's thing.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Aug 16, 2017

Member

It doesn't fail for me

Member

targos commented Aug 16, 2017

It doesn't fail for me

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 16, 2017

Member

Fail vs. not fail perhaps dependent on whether or not you are using a recent npm?

Member

Trott commented Aug 16, 2017

Fail vs. not fail perhaps dependent on whether or not you are using a recent npm?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Aug 16, 2017

Member

@MylesBorins can you post the error that you get?

Member

targos commented Aug 16, 2017

@MylesBorins can you post the error that you get?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 16, 2017

Member

The script currently assumes that there is a package.json in eslint-tmp

That is certainly incorrect as the empty directory is created just a few lines above. If anything, the script assumes the directory is empty.

I just tested with a few npm versions going back to 2.15.11 and can't replicate a problem. Definitely interested in seeing the error you're getting...

Member

Trott commented Aug 16, 2017

The script currently assumes that there is a package.json in eslint-tmp

That is certainly incorrect as the empty directory is created just a few lines above. If anything, the script assumes the directory is empty.

I just tested with a few npm versions going back to 2.15.11 and can't replicate a problem. Definitely interested in seeing the error you're getting...

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Aug 16, 2017

Member

npm's default behavior is to search up the file system for a package.json, I probably have an erroneous package.json somewhere on the filesystem.

I think it is good to be explicit here

Member

MylesBorins commented Aug 16, 2017

npm's default behavior is to search up the file system for a package.json, I probably have an erroneous package.json somewhere on the filesystem.

I think it is good to be explicit here

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 16, 2017

Member

Confirmed!

Member

Trott commented Aug 16, 2017

Confirmed!

@Trott

Trott approved these changes Aug 16, 2017

LGTM.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Aug 16, 2017

Member

Does it help to run the command with --no-save ?

Member

targos commented Aug 16, 2017

Does it help to run the command with --no-save ?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Aug 16, 2017

Member
Member

MylesBorins commented Aug 16, 2017

@targos

targos approved these changes Aug 17, 2017

MylesBorins added a commit to MylesBorins/node that referenced this pull request Aug 18, 2017

tools: fix update-eslint.sh
The script currently assumes that there is a package.json in
`eslint-tmp`. If there isn't the logic of the script fails.
This adds a call to `npm init --yes` ensuring there is a package.json
and that the script can do it's thing.

PR-URL: nodejs#14850
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
tools: fix update-eslint.sh
The script currently assumes that there is a package.json in
`eslint-tmp`. If there isn't the logic of the script fails.
This adds a call to `npm init --yes` ensuring there is a package.json
and that the script can do it's thing.

PR-URL: #14850
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Aug 18, 2017

Member

landed in fb73256

Member

MylesBorins commented Aug 18, 2017

landed in fb73256

@MylesBorins MylesBorins merged commit fb73256 into nodejs:master Aug 18, 2017

MylesBorins added a commit that referenced this pull request Sep 10, 2017

tools: fix update-eslint.sh
The script currently assumes that there is a package.json in
`eslint-tmp`. If there isn't the logic of the script fails.
This adds a call to `npm init --yes` ensuring there is a package.json
and that the script can do it's thing.

PR-URL: #14850
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

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

tools: fix update-eslint.sh
The script currently assumes that there is a package.json in
`eslint-tmp`. If there isn't the logic of the script fails.
This adds a call to `npm init --yes` ensuring there is a package.json
and that the script can do it's thing.

PR-URL: #14850
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

tools: fix update-eslint.sh
The script currently assumes that there is a package.json in
`eslint-tmp`. If there isn't the logic of the script fails.
This adds a call to `npm init --yes` ensuring there is a package.json
and that the script can do it's thing.

PR-URL: #14850
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

@MylesBorins MylesBorins deleted the MylesBorins:fix-update-eslint branch Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment