Issue 217343004: Add a "pub dependencies" command.

Issue 217343004: Add a "pub dependencies" command. (Closed)

Created:
6 years, 9 months ago by Bob Nystrom

Modified:
6 years, 8 months ago

Reviewers:
nweiz

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

Description

Add a "pub dependencies" command. BUG=https://code.google.com/p/dart/issues/detail?id=16909 RELNOTE=Add a "pub deps" command to show package dependencies. R=nweiz@google.com Committed: https://code.google.com/p/dart/source/detail?r=34662

Patch Set 1 #

Total comments: 43

Patch Set 2 : Revise. #

Total comments: 2

Created: 6 years, 8 months ago

	Unified diffs	Side-by-side diffs	Stats (+646 lines, -315 lines)
A +	sdk/lib/_internal/pub/lib/src/ascii_tree.dart	View	6 chunks	+58 lines, -23 lines	2 comments
M	sdk/lib/_internal/pub/lib/src/command.dart	View	3 chunks	+2 lines, -1 line	0 comments
A	sdk/lib/_internal/pub/lib/src/command/deps.dart	View	1 chunk	+183 lines, -0 lines	0 comments
M	sdk/lib/_internal/pub/lib/src/command/lish.dart	View	2 chunks	+2 lines, -2 lines	0 comments
D	sdk/lib/_internal/pub/lib/src/directory_tree.dart	View	1 chunk	+0 lines, -134 lines	0 comments
M	sdk/lib/_internal/pub/lib/src/utils.dart	View	1 chunk	+3 lines, -3 lines	0 comments
A	sdk/lib/_internal/pub/test/ascii_tree_test.dart	View	1 chunk	+248 lines, -0 lines	0 comments
A	sdk/lib/_internal/pub/test/deps_test.dart	View	1 chunk	+141 lines, -0 lines	0 comments
D	sdk/lib/_internal/pub/test/directory_tree_test.dart	View	1 chunk	+0 lines, -146 lines	0 comments
M	sdk/lib/_internal/pub/test/pub_test.dart	View	7 chunks	+9 lines, -6 lines	0 comments

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

nweiz

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/src/ascii_tree.dart File sdk/lib/_internal/pub/lib/src/ascii_tree.dart (right): https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/src/ascii_tree.dart#newcode75 sdk/lib/_internal/pub/lib/src/ascii_tree.dart:75: return buffer.toString(); It's weird that [_draw] doesn't initialize its ...

6 years, 8 months ago (2014-03-31 21:27:30 UTC) #2

Bob Nystrom

6 years, 8 months ago (2014-04-01 19:34:28 UTC) #3

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/ascii_tree.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/ascii_tree.dart:75: return buffer.toString();
On 2014/03/31 21:27:31, nweiz wrote:
> It's weird that [_draw] doesn't initialize its own StringBuffer, especially
now
> that it's being called multiple times.

It calls itself recursively, and we don't want to create multiple buffers.

> Also, should this maybe just call [treeFromMap] rather than calling [draw]
> directly?

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/ascii_tree.dart:99: String treeFromMap(Map map) {
On 2014/03/31 21:27:31, nweiz wrote:
> It would be nice if this took [showAllChildren] as well.

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/ascii_tree.dart:127: void _draw(StringBuffer
buffer, String prefix, String name, Map children,
On 2014/03/31 21:27:31, nweiz wrote:
> It feels like [prefix] and [name] should be named parameters as well.

I can do that, but it feels like overkill to me. It would mean either making
children named too, or moving it up in the parameter list. I like how they're
ordered now because they go based on how they're rendered: prefix then name then
children.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:246: 'dependencies': new
DepsCommand(),
On 2014/03/31 21:27:31, nweiz wrote:
> I don't like making the default name "dependencies". It's long, it's easy to
> misspell, and it sticks out a lot in the help documentation. I think we should
> just make it "deps".

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/deps.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:20: List<String> get aliases =>
const ["deps"];
On 2014/03/31 21:27:31, nweiz wrote:
> Definitely add "tab" here :).

Done. :)

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:21: String get usage => "pub
dependencies";
On 2014/03/31 21:27:31, nweiz wrote:
> "pub deps"

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:23: bool get reverse =>
commandOptions["reverse"];
On 2014/03/31 21:27:31, nweiz wrote:
> This isn't used.

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:29: defaultsTo: "text");
On 2014/03/31 21:27:31, nweiz wrote:
> This isn't used either.

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:58: /// line.
On 2014/03/31 21:27:31, nweiz wrote:
> I don't know that this is usefully different than just list mode. I'd probably
> leave it out for now to give users fewer things to decide, and since packages
> with a lot of dependencies look pretty gross in compact mode.

It's specifically what Lars requested, and it is a good bit more visually terse
than "list". This can fit most packages on a single page/screen.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:60: outputPackages(String
section, Iterable<String> names) {
On 2014/03/31 21:27:31, nweiz wrote:
> Usually we don't type-annotate local variables. If you feel these need to be
> annotated to be clear I'd move them into an external helper method. Same for
> functions in [_outputList].

I don't annotate lambdas (unnamed fns), but I'm not averse to annotating local
function declarations. I don't want to hoist them since they are conceptually
local to a single display style.

I could ditch the annotations, but I kind of like them here.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:122: outputSection("dependencies
overrides", root.dependencyOverrides);
On 2014/03/31 21:27:31, nweiz wrote:
> "dependencies" -> "dependency"

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:126: if (transitive.isNotEmpty)
{
On 2014/03/31 21:27:31, nweiz wrote:
> Nit: short-circuit

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:138: }
On 2014/03/31 21:27:31, nweiz wrote:
> It's a little confusing that this re-implements [outputSection]. What if you
did
> instead:
> 
>     var transitive = ordered(_getTransitiveDependencies(graph))
>         .map((name) => graph.packages[name]);
>     outputSection("transitive dependencies", transitive);

Done, more or less. I have to map in the above outputSection calls to go from
PackageDep -> String, which is why I didn't do this at first, but it's cleaner
the way you suggested.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:149: var toWalk = new Queue();
On 2014/03/31 21:27:31, nweiz wrote:
> The format of this queue and the way it's used is moderately complicated, so
it
> would be nice to have some documentation of it here.

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:155: toWalk.add([packageTree,
graph.packages[dep.name]]);
On 2014/03/31 21:27:31, nweiz wrote:
> You know, we do *have* a Pair type in pub.

Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:158: // Do a depth-first walk to
the dependency graph.
On 2014/03/31 21:27:31, nweiz wrote:
> I think this is actually breadth-first.

Yes! Done.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:181: }
On 2014/03/31 21:27:31, nweiz wrote:
> I like that the version constraints are visible in the list style, and I wish
> they were visible in the tree style as well. What do you think about something
> like this:
> 
>     app 0.0.0
>     |-- archive 1.0.16 <gray>(>=1.0.0 <2.0.0)</gray>
>     |   '-- crypto... (>=0.9.0 <1.0.0)

I tried that, but ended up not liking it. It was quite cluttered, and the tree
view is already pretty... wordy.

If users want to see the constraints, I think the list view is a better fit
anyway. The tree is best for showing the dependency chains.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/lish.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/lish.dart:106: '${treeFromFiles(files,
baseDir: entrypoint.root.dir)}');
On 2014/03/31 21:27:31, nweiz wrote:
> Maybe we should start importing ascii_tree with a prefix, so we can do
> "tree.fromFiles" and "tree.generate".

Done.

nweiz

lgtm https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/src/ascii_tree.dart File sdk/lib/_internal/pub/lib/src/ascii_tree.dart (right): https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/src/ascii_tree.dart#newcode127 sdk/lib/_internal/pub/lib/src/ascii_tree.dart:127: void _draw(StringBuffer buffer, String prefix, String name, Map ...

6 years, 8 months ago (2014-04-02 03:11:12 UTC) #4

lgtm

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/ascii_tree.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/ascii_tree.dart:127: void _draw(StringBuffer
buffer, String prefix, String name, Map children,
On 2014/04/01 19:34:28, Bob Nystrom wrote:
> On 2014/03/31 21:27:31, nweiz wrote:
> > It feels like [prefix] and [name] should be named parameters as well.
> 
> I can do that, but it feels like overkill to me. It would mean either making
> children named too, or moving it up in the parameter list. I like how they're
> ordered now because they go based on how they're rendered: prefix then name
then
> children.

I won't push, but it seems weird to have parameters with default values ("" and
null as called in [treeFromMap]) but not make them named.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/deps.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:58: /// line.
On 2014/04/01 19:34:28, Bob Nystrom wrote:
> On 2014/03/31 21:27:31, nweiz wrote:
> > I don't know that this is usefully different than just list mode. I'd
probably
> > leave it out for now to give users fewer things to decide, and since
packages
> > with a lot of dependencies look pretty gross in compact mode.
> 
> It's specifically what Lars requested

I don't think this is a good reason to include it. Lars wanted some sort of
visualization; I don't think he cares about the details of what it looks like,
nor is he the best person to design them.

> and it is a good bit more visually terse than "list". This can fit most
packages on a single page/screen.

Granted I have a large screen, but in my local tests, I wasn't unhappy with the
size of the "list" output. In fact, what I found more visually annoying was the
long lines of "compact" wrapping; wrapped lines are very unpleasant in
terminals.

If you think users will actually get value from this existing, go ahead and keep
it, but I don't think they will.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:60: outputPackages(String
section, Iterable<String> names) {
On 2014/04/01 19:34:28, Bob Nystrom wrote:
> On 2014/03/31 21:27:31, nweiz wrote:
> > Usually we don't type-annotate local variables. If you feel these need to be
> > annotated to be clear I'd move them into an external helper method. Same for
> > functions in [_outputList].
> 
> I don't annotate lambdas (unnamed fns), but I'm not averse to annotating local
> function declarations. I don't want to hoist them since they are conceptually
> local to a single display style.
> 
> I could ditch the annotations, but I kind of like them here.

I very much feel that all local declarations should be non-annotated. Function
bodies tend to be subject to a lot more churn than function boundaries, and they
tend to be more token-dense as well; annotations make both of these more
difficult. In addition, I really like using "I can't figure out what type this
is" as a heuristic to factor out sub-functions.

As to hoisting, we frequently have non-local helper methods that are specific to
a single parent function. I like that more than local functions unless the local
function is doing almost the same thing as the parent function (e.g. for
recursion); it gives the author an opportunity to define and document what the
helper function is doing, what its calling conventions are, etc.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:181: }
On 2014/04/01 19:34:28, Bob Nystrom wrote:
> On 2014/03/31 21:27:31, nweiz wrote:
> > I like that the version constraints are visible in the list style, and I
wish
> > they were visible in the tree style as well. What do you think about
something
> > like this:
> > 
> >     app 0.0.0
> >     |-- archive 1.0.16 <gray>(>=1.0.0 <2.0.0)</gray>
> >     |   '-- crypto... (>=0.9.0 <1.0.0)
> 
> I tried that, but ended up not liking it. It was quite cluttered, and the tree
> view is already pretty... wordy.
> 
> If users want to see the constraints, I think the list view is a better fit
> anyway. The tree is best for showing the dependency chains.

SGTM

https://codereview.chromium.org/217343004/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/ascii_tree.dart (right):

https://codereview.chromium.org/217343004/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/ascii_tree.dart:64: bool showAllChildren}) {
Indent +2.

I still prefer always using default values for named booleans, but YMMV.

Bob Nystrom

Committed patchset #2 manually as r34662 (presubmit successful).

6 years, 8 months ago (2014-04-02 19:18:31 UTC) #5

Bob Nystrom

6 years, 8 months ago (2014-04-02 19:18:37 UTC) #6

Message was sent while issue was closed.

Thanks!

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/ascii_tree.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/ascii_tree.dart:127: void _draw(StringBuffer
buffer, String prefix, String name, Map children,
On 2014/04/02 03:11:12, nweiz wrote:
> On 2014/04/01 19:34:28, Bob Nystrom wrote:
> > On 2014/03/31 21:27:31, nweiz wrote:
> > > It feels like [prefix] and [name] should be named parameters as well.
> > 
> > I can do that, but it feels like overkill to me. It would mean either making
> > children named too, or moving it up in the parameter list. I like how
they're
> > ordered now because they go based on how they're rendered: prefix then name
> then
> > children.
> 
> I won't push, but it seems weird to have parameters with default values (""
and
> null as called in [treeFromMap]) but not make them named.

Yeah, I see where you're coming from, but I just don't think of them as
defaults. They're mandatory arguments.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/deps.dart (right):

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:58: /// line.
On 2014/04/02 03:11:12, nweiz wrote:
> On 2014/04/01 19:34:28, Bob Nystrom wrote:
> > On 2014/03/31 21:27:31, nweiz wrote:
> > > I don't know that this is usefully different than just list mode. I'd
> probably
> > > leave it out for now to give users fewer things to decide, and since
> packages
> > > with a lot of dependencies look pretty gross in compact mode.
> > 
> > It's specifically what Lars requested
> 
> I don't think this is a good reason to include it. Lars wanted some sort of
> visualization; I don't think he cares about the details of what it looks like,
> nor is he the best person to design them.

Well, he did include specific example output in the bug and sent a patch that
implemented it.

> 
> > and it is a good bit more visually terse than "list". This can fit most
> packages on a single page/screen.
> 
> Granted I have a large screen, but in my local tests, I wasn't unhappy with
the
> size of the "list" output. In fact, what I found more visually annoying was
the
> long lines of "compact" wrapping; wrapped lines are very unpleasant in
> terminals.
> 
> If you think users will actually get value from this existing, go ahead and
keep
> it, but I don't think they will.

I actually do like it.

https://codereview.chromium.org/217343004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/deps.dart:60: outputPackages(String
section, Iterable<String> names) {
On 2014/04/02 03:11:12, nweiz wrote:
> On 2014/04/01 19:34:28, Bob Nystrom wrote:
> > On 2014/03/31 21:27:31, nweiz wrote:
> > > Usually we don't type-annotate local variables. If you feel these need to
be
> > > annotated to be clear I'd move them into an external helper method. Same
for
> > > functions in [_outputList].
> > 
> > I don't annotate lambdas (unnamed fns), but I'm not averse to annotating
local
> > function declarations. I don't want to hoist them since they are
conceptually
> > local to a single display style.
> > 
> > I could ditch the annotations, but I kind of like them here.
> 
> I very much feel that all local declarations should be non-annotated. Function
> bodies tend to be subject to a lot more churn than function boundaries, and
they
> tend to be more token-dense as well; annotations make both of these more
> difficult. In addition, I really like using "I can't figure out what type this
> is" as a heuristic to factor out sub-functions.
> 
> As to hoisting, we frequently have non-local helper methods that are specific
to
> a single parent function. I like that more than local functions unless the
local
> function is doing almost the same thing as the parent function (e.g. for
> recursion); it gives the author an opportunity to define and document what the
> helper function is doing, what its calling conventions are, etc.

I agree with all of this.

At the same time, I also sometimes like large-ish local functions. They are big
enough to have their own annotations, but are still useful as local functions.
It lets them easily close over stuff in the parent function instead of having to
pass a handful of parameters around. I treat the outer function as sort of a
poor man's class: a bag of state that is implicitly shared over the local
function (and any other peer local fns).

I hoisted these functions out like you requested and documented them. To avoid
passing around the StringBuffer and PackageGraph to all of these functions, I
move those to be members of the surrounding class. That feels a bit dirty to me
because those members only exist at certain points in time (after the graph has
been loaded). Closing over them in local functions fixes that problem while
still making them implicitly available to those functions.

https://codereview.chromium.org/217343004/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/ascii_tree.dart (right):

https://codereview.chromium.org/217343004/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/ascii_tree.dart:64: bool showAllChildren}) {
On 2014/04/02 03:11:12, nweiz wrote:
> Indent +2.
> 
> I still prefer always using default values for named booleans, but YMMV.

I don't want to repeat the default here and in _draw.

Expand Messages | Collapse Messages