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.

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

fix: wrong npm doctor command result #1416

Closed
wants to merge 3 commits into from
Closed

fix: wrong npm doctor command result #1416

wants to merge 3 commits into from

Conversation

vanishcode
Copy link
Contributor

Detail

when I run npm doctor, I found that the some infomation in the result table were totaly wrong, look the screenshot:

kvB9iFG6ApaOdtP

look at the Recommendation column

the right result:

YkHKdQ96gLieF1y

I found that if I changed the default registry to another url (such as https://registry.npm.taobao.org), and ping returns error, it would happend.

so i checked and modified the check-ping.js, when ping is failed, return cb(null, [err.code.substr(1), 'failed']) instead of cb(null, [err.code.substr(1)])

i don't know if it's a bug certainty, if it's realy not, i'll close this pr.

(forgive me that English is not my mother language······)

Environment

macOS 10.15.5
node 10.18.0
npm 6.14.5 (from source code)

@vanishcode vanishcode requested a review from a team as a code owner June 10, 2020 11:35
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

hi @vanishcode thanks for the contribution 😊 this looks like a good fix! we would need some unit tests that could help us reproduce the error and that we can use it to validate the fix solves it - you can take a look at https://github.com/npm/cli/blob/latest/test/tap/doctor.js for reference (and that would also be the perfect place to add a new test).

@ruyadorno ruyadorno added the pr: needs tests requires tests before merging label Jun 12, 2020
@vanishcode
Copy link
Contributor Author

ok, I will add unit test for this fix later. thank you! @ruyadorno

Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants