Skip to content
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

dotnuet nuget why uses current directory when provided with one argument #5969

Merged
merged 11 commits into from
Aug 17, 2024

Conversation

zivkan
Copy link
Member

@zivkan zivkan commented Aug 15, 2024

Bug

Fixes: NuGet/Home#13553

Description

Big thanks to @baronfel for the advice on how to get this to work.

Change the path argument to use a custom parser, where we can tell System.CommandLine if we want to take zero or one tokens from the parsed argument list. When we take zero, we use the current directory as a default.

I also made why's command line parsing configuration unit testable, so we can easily and exhaustively test all scenarios we care about, and it's still super fast (compared to invoking dotnet nuget why in dotnet.integration.tests, waiting for the process to end, and checking the exit code).

Also moved some code and namespaces around, to make things more intuitive/how other projects organise files and namespaces.
 

PR Checklist

@zivkan zivkan requested a review from a team as a code owner August 15, 2024 00:20
nkolev92
nkolev92 previously approved these changes Aug 15, 2024
@@ -21,9 +21,9 @@

<!-- Microsoft.Build.Locator is only used when debugging, and the compiler will skip copying this dependency from Release assemblies we insert because we only refer to it conditionally with the DEBUG configuration.
Uncomment the following when debugging. Also uncomment the MSBuildLocator code from Program.cs -->
<!-- <ItemGroup>
<!--<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this change?

I still like it when we had this under a Choose/When condition since it was just easy to work with, but that's not the topic of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't need that change. I made such an effort to avoid commiting this file (and uncommenting the related lines in program.cs) on each commit, but all it took was one slip-up ☹️ Then, instead of "reverting" the file, I tried using the IDE to comment out the same lines, but it didn't do it the same way as the code before.

I also prefer the choose-when for debug mode. I don't know what problem was trying to be solved by having debug and release builds work the same, but in my experience working on this bug, it makes development harder. The debug-only code was just easier to work with and had no impact on the optimized binaries that we ship to customers.

Nigusu-Allehu
Nigusu-Allehu previously approved these changes Aug 15, 2024
@zivkan zivkan dismissed stale reviews from Nigusu-Allehu and nkolev92 via 849a536 August 16, 2024 04:02
@zivkan zivkan merged commit 0539e8e into dev Aug 17, 2024
28 checks passed
@zivkan zivkan deleted the dev-zivkan-dotnet-nuget-why-default-directory branch August 17, 2024 22:30
@zivkan zivkan added this to the 6.12 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dotnet nuget why shouldn't require project when there is only one in the current directory
3 participants