add depth specification to ls. Fixes #726 #1463

olingern · 2016-10-25T23:41:13Z

Summary
Per #726, filtering the depth of children displayed should be supported.

Test plan
Added support for: yarn ls depth [LEVEL] where the first level is 1.

Examples:

yarn ls depth 1

yarn ls depth 2

`

wyze · 2016-10-26T00:45:23Z

src/cli/commands/ls.js

@@ -45,7 +45,7 @@ export async function buildTree(
  const treesByKey = {};
  const trees = [];
  const hoisted = await linker.getFlatHoistedTree(patterns);
-
+  //console.log(hoisted);


Commented out console.log snuck in there.

Indeed! When I fully understand the use case of yarn ls that I mentioned in #726, I'll add tests, remove this log, and rebase

wyze · 2016-10-26T00:46:19Z

npm ls command starts with depth 0 being the top level dependencies. Should we match that here? Right now you have it as starting from 1.

olingern · 2016-10-26T01:40:45Z

@wyze I could. I considered 0 to be a non-depth and it circumvents filtering, i.e. yarn ls

Zero-indexing the tree structure shouldn't be too hard, just handle no-filtering differently

hawkrives · 2016-10-26T15:52:22Z

It's always bugged me that npm used 0-indexing for --depth — especially since the du command is 1-indexed, and I wind up using them together a lot.

I would appreciate it if yarn used 1-indexing, but I'll deal with 0-indexing 😉 (also I have no actual vote in this matter, since I've not written any code here.)

olingern · 2016-10-26T21:55:19Z

@hawkrives @wyze Obviously, I like 1-indexed, but I'm not sure if the goal is to provide a CLI that is synonymous with npm's, or just "inspired."

samccone · 2016-10-26T22:22:05Z

I vote Lets match npm here

olingern · 2016-10-27T14:47:54Z

@wyze @samccone Depth is now indexed at 0. I've also added support for a few other ls options. Hopefully, a good direction to go in 😄

Below are the commands supported in this PR
yarn ls --depth [LEVEL]
yarn ls --prod show dependencies only
yarn ls --la / yarn ls --long yarn ls --ll show detailed tree view

Detailed tree view was probably the most fun, and difficult to get working (in an elegant way), but it lacks description and repo url that npm has
Will be adding tests for all updates in this commit over the next day. Please, feel free to give any feedback.

steelbrain · 2016-10-27T16:11:46Z

.vscode/settings.json

+// Place your settings in this file to overwrite default and user settings.
+{
+    "javascript.validate.enable": false
+}


sebmck · 2016-10-27T20:40:16Z

src/cli/commands/ls.js

+
+export function setFlags(commander: Object) {
+  commander.option('--depth [depth]', 'depth');
+  commander.option('--prod [prod]', 'prod');


Can you add some descriptions to these options? It's not really obvious what they're supposed to do.

sebmck · 2016-10-27T20:40:59Z

src/reporters/console/console-reporter.js

+  extendedTree(key: string, trees: Trees) {
+    // 
+    const output = ({name, type, registry, reference, children, hint, color}, level, end) => {
+      const _this = this;


What's the purpose of declaring this?

This adds support for yarn ls --long or yarn ls --la or yarn ls --ll, which is a nod to npm's npm ls --long

The two paths of outputting a tree diverge enough that I needed to break the original tree function into smaller bits that could be consumed by another function, extendedTree you see here

olingern · 2016-10-31T04:11:15Z

@kittens @wyze To keep this moving along, I reworked this PR to only include work for the yarn ls --depth=[LEVEL] command. The scope of the commit should be much smaller 😉

wyze · 2016-11-04T22:35:22Z

src/cli/commands/add.js

@@ -4,6 +4,7 @@ import type {Reporter} from '../../reporters/index.js';
 import type {InstallCwdRequest, InstallPrepared} from './install.js';
 import type {DependencyRequestPatterns} from '../../types.js';
 import type Config from '../../config.js';
+import type{lsOptions} from './ls.js';


Add a space between type and {.

wyze · 2016-11-04T22:36:05Z

src/reporters/types.js

@@ -11,6 +11,7 @@ export type Package = {
  version: string
 };

+


Remove extra newline.

wyze · 2016-11-04T22:36:27Z

src/reporters/console/helpers/tree-helper.js

+// types
+import type {
+  Trees,
+} from '../../types.js';


This should be on one line.

wyze · 2016-11-04T22:37:21Z

src/reporters/console/helpers/tree-helper.js

+  Trees,
+} from '../../types.js';
+
+export type formattedOutput = {


Type name should begin with a capitol letter.

wyze · 2016-11-04T22:38:10Z

src/reporters/console/helpers/tree-helper.js

+  const _item = formatColor(fmt.color, fmt.name, fmt.formatter);
+  const _indent = getIndent(fmt.end, fmt.level);
+  const _suffix = getSuffix(fmt.hint, fmt.formatter);
+  return `${_indent}─ ${_item}${_suffix}\n`;


Remove the _ from the variables here.

wyze · 2016-11-04T22:40:01Z

src/cli/commands/ls.js

@@ -11,7 +11,10 @@ import Lockfile from '../../lockfile/wrapper.js';
 const invariant = require('invariant');

 export const requireLockfile = true;
-export const noArguments = true;
+
+export type lsOptions = {


Capitalize the type here too.

wyze · 2016-11-04T22:41:05Z

@olingern Please rebase and make the requested changes and see if we can land this!

olingern · 2016-11-05T00:34:56Z

@wyze Will do shortly! Thanks for the feedback!

olingern · 2016-11-07T13:00:30Z

@wyze Changes made, along with one logic fix. My tests pass locally, I'm not sure what's up with travis-ci and appveyor.

wyze · 2016-11-07T13:46:56Z

@olingern, I'm not sure if some stuff landed or when you did the rebase, but I just did a fresh rebase on your branch and resolved the conflicts and pushed up. If everything comes back green, we are good to go.

wyze · 2016-11-07T14:09:38Z

Thanks again for all the iteration here @olingern! Would you mind opening a PR over on yarnpkg/website to add documentation for this?

olingern · 2016-11-08T01:02:30Z

@wyze No problem!

PR is open here: #251.

wyze reviewed Oct 26, 2016

View reviewed changes

samccone assigned wyze Oct 26, 2016

steelbrain reviewed Oct 27, 2016

View reviewed changes

sebmck reviewed Oct 27, 2016

View reviewed changes

olingern mentioned this pull request Oct 27, 2016

extra color for bolded text in the output #1123

Open

wyze suggested changes Nov 4, 2016

View reviewed changes

add depth specification to ls

d352349

wyze approved these changes Nov 7, 2016

View reviewed changes

wyze merged commit 253269f into yarnpkg:master Nov 7, 2016

wyze mentioned this pull request Nov 7, 2016

Add max depth filter in yarn ls #726

Closed

olingern mentioned this pull request Nov 8, 2016

Add documentation for ls and ls --depth command yarnpkg/website#251

Merged

bestander pushed a commit to bestander/yarn that referenced this pull request Nov 9, 2016

add depth specification to ls (yarnpkg#1463)

d9990e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add depth specification to ls. Fixes #726 #1463

add depth specification to ls. Fixes #726 #1463

olingern commented Oct 25, 2016 •

edited

Loading

wyze Oct 26, 2016

olingern Oct 26, 2016

wyze commented Oct 26, 2016

olingern commented Oct 26, 2016

hawkrives commented Oct 26, 2016

olingern commented Oct 26, 2016

samccone commented Oct 26, 2016

olingern commented Oct 27, 2016 •

edited

Loading

steelbrain Oct 27, 2016

sebmck Oct 27, 2016

olingern Oct 27, 2016

sebmck Oct 27, 2016

olingern Oct 27, 2016 •

edited

Loading

olingern commented Oct 31, 2016 •

edited

Loading

wyze Nov 4, 2016

wyze Nov 4, 2016

wyze Nov 4, 2016

wyze Nov 4, 2016

wyze Nov 4, 2016

wyze Nov 4, 2016

wyze commented Nov 4, 2016 •

edited

Loading

olingern commented Nov 5, 2016

olingern commented Nov 7, 2016

wyze commented Nov 7, 2016

wyze commented Nov 7, 2016

olingern commented Nov 8, 2016 •

edited

Loading

add depth specification to ls. Fixes #726 #1463

add depth specification to ls. Fixes #726 #1463

Conversation

olingern commented Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyze commented Oct 26, 2016

olingern commented Oct 26, 2016

hawkrives commented Oct 26, 2016

olingern commented Oct 26, 2016

samccone commented Oct 26, 2016

olingern commented Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olingern Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

olingern commented Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyze commented Nov 4, 2016 • edited Loading

olingern commented Nov 5, 2016

olingern commented Nov 7, 2016

wyze commented Nov 7, 2016

wyze commented Nov 7, 2016

olingern commented Nov 8, 2016 • edited Loading

olingern commented Oct 25, 2016 •

edited

Loading

olingern commented Oct 27, 2016 •

edited

Loading

olingern Oct 27, 2016 •

edited

Loading

olingern commented Oct 31, 2016 •

edited

Loading

wyze commented Nov 4, 2016 •

edited

Loading

olingern commented Nov 8, 2016 •

edited

Loading