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

Fix ls depth when outputting json #10505

Closed
wants to merge 2 commits into from
Closed

Conversation

MarkReeder
Copy link
Contributor

This fixes the depth option when used in combination with the json option.

As an example, prior to this diff:

npm ls -depth=0

r-dom@2.1.0 /root/temp/r-dom
├── classnames@1.2.2
├── for-each@0.3.2
...

npm ls -depth=0 --json

{
  "name": "r-dom",
  "version": "2.1.0",
  "dependencies": {
    "classnames": {
      "version": "1.2.2",
      "from": "classnames@^1.1.4",
      "resolved": "https://registry.npmjs.org/classnames/-/classnames-1.2.2.tgz"
    },
    "for-each": {
      "version": "0.3.2",
      "from": "for-each@^0.3.2",
      "resolved": "https://registry.npmjs.org/for-each/-/for-each-0.3.2.tgz",
      "dependencies": {
        "is-function": {
          "version": "1.0.1",
          "from": "is-function@~1.0.0",
          "resolved": "https://registry.npmjs.org/is-function/-/is-function-1.0.1.tgz"
        }
      }
    },
...

and after this change:

npm ls -depth=0

r-dom@2.1.0 /root/temp/r-dom
├── classnames@1.2.2
├── for-each@0.3.2
...

npm ls -depth=0 --json

{
  "name": "r-dom",
  "version": "2.1.0",
  "dependencies": {
    "classnames": {
      "version": "1.2.2",
      "from": "classnames@^1.1.4",
      "resolved": "https://registry.npmjs.org/classnames/-/classnames-1.2.2.tgz"
    },
    "for-each": {
      "version": "0.3.2",
      "from": "for-each@^0.3.2",
      "resolved": "https://registry.npmjs.org/for-each/-/for-each-0.3.2.tgz"
    },
...

Note that the dependencies of for-each are incorrectly included prior and correctly excluded after.

@othiym23
Copy link
Contributor

This looks good! It needs a test before it can be landed, though. We're in the process of doing a bunch of work to the npm tests, and part of that will be writing a new guide on how to write a good npm test, but in the meantime, if you want to take a look at what's in test/tap and copypasta one of the existing tests, that would be a good start. Thanks for putting this together!

@MarkReeder
Copy link
Contributor Author

I added some tests a couple days ago. They're copypasta'd from similar tests in test/tap/ls-depth-cli.js - let me know if you'd like me to make any changes.

@lxe
Copy link
Contributor

lxe commented Dec 10, 2015

Ah I am observing this issue as well. Would be nice to get this merged!

othiym23 pushed a commit that referenced this pull request Dec 11, 2015
@othiym23
Copy link
Contributor

Merged and rebased and landed for npm@3 as 71c9590. Will do the same for npm@2 shortly.

othiym23 pushed a commit that referenced this pull request Dec 11, 2015
@othiym23
Copy link
Contributor

Cherry-picked for npm@2 as f482664. Thanks for putting this together, and thanks for adding the tests!

@othiym23 othiym23 closed this Dec 11, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants