-
Notifications
You must be signed in to change notification settings - Fork 228
Conversation
app.Command("list", c => | ||
{ | ||
c.Description = "Print the dependencies of a given project."; | ||
var coreclrRoot = c.Option("--coreclr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this --coreclr? and not --runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
humm may need a better name. I'm a bit hesitated on "runtime". First of all it is not used in the case of CLR framework. Second, it is actually used to point the CoreCLR packages folder rather than the runtime folder. And also put "runtime" is a little bit ambiguous, in kvm
it accepts a runtime version name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should totally be runtime. The flag in kvm is -r aka --runtime
the flag in kpm pack is also --runtime
. Of of the values of the -r flag on kvm is CoreCLR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a switch, it accept a path to the CoreCLR assemblies folder. The usage is different from the -r
and --runtime
.
I think this is insane 😄. We should look at what npm and other package manages do when listing remote packages. Maybe the default list command requires a search query and we can show results by feed.
For --local, I think you want to treat this command like a command line version of the references node in visual studio. You use it to see what your project is referencing, and to see unresolved dependencies and other information like that. Maybe if --runtime is specified, then we can walk past the package graph into the implementation, otherwise we don't show it by default. |
I would also mirror the output of pack and build when listing dependencies |
Ah, it should be:
|
nit:
It should be something like:
|
You may also want to consider showing an actual dependency tree. |
How about a reversed dependency tree: each package follows by the chain of dependencies explaining why it is introduced? |
By default? Nope, it should be an option though. This command looks promising but I think what I said holds. Assume this is the command line equivalent of the references node. Try using it that way on some projects and see if it's actually useful. |
There also needs to be a --framework flag so you can specify which framework (of the ones specified in your project.json file) to show |
|
{ | ||
foreach (var loadableAssembly in libraryInformation.LoadableAssemblies) | ||
{ | ||
IEnumerable<string> dependencies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor : can this be var dependencies = ResolveLocalDependency(loadableAssembly.Name, assemblyName => missing.Add(assemblyName));
instead?
Rebase on the latest dev. I'll begin addressing the first round of feedbacks. |
The command design is overhauled. The description will be updated. |
@davidfowl @muratg @loudej @Praburaj @ChengTian |
Also @Eilon, I used the spelling check plugin this time. Hopefully it saved you some work. |
/// <summary> | ||
/// Walk the graph in depth-first post order | ||
/// </summary> | ||
public class DFPostOrderWalker<T, K> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T
and K
as generic args is not very descriptive. Normally it would be TFoo
and TBar
, though in this case it's not clear to me what the two args represent at all. Do you have better names for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T
-> TNode
, represents the type of the graph's node
K
-> TState
, represents the type of the result of the traversal, it is a state pass down in the recursive invocation.
Advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's better than the old names, for sure 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the file name be DepthFirstPostOrderWalker.cs instead of DFPostOrderWalker.cs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about that but the name appears a little bit long. So here's my thought, I'd like to have a set of common used algorithms and data structures place together. There name can follow the convention that DF
stands for Depth First
and BF
stands for Breath First
. It helps to shorten the name without hurt the readability to much.
Advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DF
and BF
are not commonly known acronyms so I recommend against using them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right.
|
||
public void Draw(IGraphNode<Library> root) | ||
{ | ||
var dict = Collect(root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dict
? Dictionary of what?
@Eilon thanks for the comments on names/comments/messages, all accept, I'll update. |
|
||
if (set.Any()) | ||
{ | ||
line.AppendFormat(" ({0})", string.Join(",", set.Select(l => l.ToString()).OrderBy(s => s).ToArray())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l
? s
?
My PR is trending (103 comments and growing) and more changes is coming. Discussed with @davidfowl, following changes will be made to simplified the codes.
|
Func<TNode, IEnumerable<TNode>, bool> visitNode, | ||
Func<TNode, IEnumerable<TNode>> getChildren) | ||
{ | ||
if (visitNode == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use [NotNull]
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. There is no NotNull
in kpm project.
{ | ||
if (_framework != AspNetCore50) | ||
{ | ||
var gacDesc = _gacResolver.GetDescription(new Library { Name = assembly, IsGacOrFrameworkReference = true }, _framework); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was supposed to go into ResolveAssemblyFilePath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the GAC resolving is not actually needed since we ignore all the unresolved assemblies here. I'll remove the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
public void Render(IGraphNode<Library> root) | ||
{ | ||
var dict = FindImmediateDependent(root); | ||
foreach (var key in dict.Keys.OrderBy(library => library.ToString())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
library.Name? Instead of ToString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the order by
after that last change |
_framework = framework; | ||
|
||
_hostContext = CreateApplicationHostContext(); | ||
_gacResolver = new GacDependencyResolver(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still required?
0ee1f21
to
5b4883e
Compare
Add a new command to kpm:
kpm list
The command lists the dependencies of given project. The command runs in two modes.
Under local mode a path to the runtime assemblies folder is allowed optionally. If provided, the command will try to use the assemblies under that folder to resolve the dependencies.
Usage examples:
Lists all the libraries dependencies in a flat mode. Following the packages, immediate dependents are listed.
List the full paths to the assemblies referenced by the project on local disk.
List all the libraries dependencies in a tree mode. The tree is fully expanded.
List all the libraries dependencies in a tree mode, but the three will be collapsed under level 3.