Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Fix: show settings from per-project .npmrc file in "npm config list" #11475

Closed
wants to merge 1 commit into from
Closed

Fix: show settings from per-project .npmrc file in "npm config list" #11475

wants to merge 1 commit into from

Conversation

mjomble
Copy link
Contributor

@mjomble mjomble commented Feb 9, 2016

Fixes #11472

@othiym23 othiym23 added the review label Feb 9, 2016
@iarna
Copy link
Contributor

iarna commented Feb 25, 2016

I'd like to see a test for this.

@iarna iarna added needs-tests and removed review labels Feb 25, 2016
@mjomble
Copy link
Contributor Author

mjomble commented Feb 25, 2016

Ok, I can look into adding some

@mjomble
Copy link
Contributor Author

mjomble commented Mar 6, 2016

I've added a test now.

I noticed there were no tests at all for npm config list and although it would be nice to also have tests for other config types (cli, env, user, global), I felt they were outside the scope of this PR. So I only implemented a test for per-project conf for now and added a TODO comment for implementing the others.

The Travis build failed in 0.8, but it seems like a generic configuration issue that is not related to this PR. I noticed a similar failure in a different PR as well. For >= 0.10, all builds are passing.

@iarna
Copy link
Contributor

iarna commented Mar 10, 2016

The 0.8 failure is, as you assume, not related to your PR. (Ordinarily 0.8 does pass– I've flagged it for a rebuild. It's an issue we've visited earlier, and is related to a bug in 0.8's http module that we've papered over as best as we were able, unfortunately it still seems to show up, albeit much less often.)

@iarna
Copy link
Contributor

iarna commented Mar 10, 2016

(I also agree that other tests are out of scope here. They'd be fantastic, but are hardly required for this PR.)

@iarna iarna added this to the next milestone Mar 10, 2016
@mjomble
Copy link
Contributor Author

mjomble commented Mar 10, 2016

Maybe I'll look into the other tests once this PR gets merged :)

@iarna
Copy link
Contributor

iarna commented Mar 10, 2016

Thank you for putting this all together, LGTM

iarna pushed a commit that referenced this pull request Mar 10, 2016
@mjomble @iarna
config: include per-project .npmrc file in "npm config list"
cf5fd40 
Credit: @mjomble
Reviewed-By: @iarna
PR-URL: #11475
Fixes: #11472
@iarna
Copy link
Contributor

iarna commented Mar 11, 2016

This landed in 3.8.2!

@iarna iarna closed this Mar 11, 2016
michaelnisi pushed a commit to michaelnisi/npm that referenced this pull request Apr 2, 2016
@mjomble @michaelnisi
config: include per-project .npmrc file in "npm config list"
970b44f 
Credit: @mjomble
Reviewed-By: @iarna
PR-URL: npm#11475
Fixes: npm#11472
@othiym23 othiym23 mentioned this pull request Sep 16, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
merge-to-latest
Projects
None yet
Milestone
3.8.2 & 2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mjomble @iarna @othiym23