-
Notifications
You must be signed in to change notification settings - Fork 735
Add dotnet nuget why command
#5761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
donnie-msft
left a comment
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.
Really cool seeing this come together. Nice work!
test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatWhyTests.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/IWhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatWhyTests.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatWhyTests.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatWhyTests.cs
Outdated
Show resolved
Hide resolved
| string targetPackage = whyCommandArgs.Package; | ||
|
|
||
| IEnumerable<string> projectPaths = Path.GetExtension(whyCommandArgs.Path).Equals(".sln") | ||
| ? MSBuildAPIUtility.GetProjectsFromSolution(whyCommandArgs.Path).Where(f => File.Exists(f)) |
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.
I'm guessing that MSBuild only returns project files which exist. @jeffkl would know for sure.
|
|
||
| foreach (var projectPath in projectPaths) | ||
| { | ||
| Project project = MSBuildAPIUtility.GetProject(projectPath); |
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.
Maybe we can enhance the MSBuildAPIUtility to just return the ProjectAndSolution type instead of selecting only the paths in GetProjectsFromSolution. That way, you wouldn't need convert the paths back to projects, here.
I'm not familiar with these types, so let's see if @jeffkl has opinions on this.
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
kartheekp-ms
left a comment
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.
I am still reviewing this PR but left couple of comments to begin with.
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Show resolved
Hide resolved
test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatWhyTests.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
....Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandUtility/DependencyGraphFinder.cs
Outdated
Show resolved
Hide resolved
....Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandUtility/DependencyGraphFinder.cs
Outdated
Show resolved
Hide resolved
....Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandUtility/DependencyGraphFinder.cs
Outdated
Show resolved
Hide resolved
....Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandUtility/DependencyGraphFinder.cs
Show resolved
Hide resolved
....Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandUtility/DependencyGraphFinder.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/DependencyNode.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandRunner.cs
Outdated
Show resolved
Hide resolved
....Core/NuGet.CommandLine.XPlat/Commands/WhyCommand/WhyCommandUtility/DependencyGraphFinder.cs
Outdated
Show resolved
Hide resolved
zivkan
left a comment
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.
I forgot to submit these comments last week! Maybe some of them are no longer relevant due to updates made since I wrote the comments locally.
| using NuGet.CommandLine.XPlat.WhyCommandUtility; | ||
| using NuGet.ProjectModel; | ||
|
|
||
| namespace NuGet.CommandLine.XPlat |
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.
nitpick: Just wondering why the namespace is different to the project's default namespace + directory structure?
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 commands and files in this project fall in the NuGet.CommandLine.XPlat namespace, so I've removed other namespaces like NuGet.CommandLine.XPlat.WhyCommandUtility and moved everything under NuGet.CommandLine.XPlat to keep things consistent. Let me know if you think this needs changes or can be improved upon
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.
IMO the reasons to organize code on the filesystem is identical to the reasons to organize code in namespaces. Therefore, I consider it bad when they're out of sync. But I recognise that NuGet's existing code does this. I just think it's a bad pattern that we should stop copying. But I'm not going to hold up this PR because of this, and I don't know if anyone else in the team share my opinion. But for what it's worth, since this file's class is internal, there's no public API risk of moving it to a "more appropriate" namespace.
Bug
Fixes: NuGet/Home#11943
Regression? Last working version:
Description
Design spec: dotnet nuget why command
Adds a new
dotnet nuget whycommand to the Dotnet CLI, allowing users to see the dependency graph for a given package. They can use this to easily identify the top-level packages that are bringing in a given transitive dependency.dotnet nuget why -hdotnet nuget why C:\TestProject.sln System.Text.JsonThis was built on the proof-of-concept made by Pragnya and Kartheek: pragnya17-dotnet-nuget-why
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation
dotnet nuget whyHome#13499