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

Add dotnet package search #5466

Conversation

Nigusu-Allehu
Copy link
Contributor

@Nigusu-Allehu Nigusu-Allehu commented Oct 18, 2023

Bug

Fixes: NuGet/Home#6060

Regression? Last working version:

Description

ℹ️ Note : This PR is merged into dev-feature-migrate-systemcommandline, as there is some migration work to be done.

image

PR Checklist

@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner October 18, 2023 17:00
@Nigusu-Allehu Nigusu-Allehu marked this pull request as draft October 18, 2023 17:00
@Nigusu-Allehu Nigusu-Allehu changed the title Dev nyenework dotnet package search Add dotnet package search Oct 18, 2023
@Nigusu-Allehu Nigusu-Allehu self-assigned this Oct 18, 2023
@Nigusu-Allehu Nigusu-Allehu marked this pull request as ready for review October 19, 2023 22:58
Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Very meticulous work here, good job!
I'm not completely done reviewing, but needed to submit what I have so far.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Looks great!

Most of my feedback is minor.
I think we have some classes for which we can add some more unit tests.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Excellent job!

I do have 1 question, but addressing that feedback in a future PR is fine with me.

Given that this is a very large PR, I'd try to get approval from more than just 1 person.

}

packageSearchResultRenderer.Finish();
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Say we have 2 sources, only 1 responds, but 1 times out. Do we want the exit to be 0?
It's a genuine question. Have you checked how nuget.exe list or nuget.exe handle that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I hope we would still wait for and display results from other feeds in this case. For internal projects we often have many feeds and who knows whether one will have a problem for some reason..

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I agree, and it seems like the implementation is doing that as well (displaying partial results).

Just curious what should the exit code would be when we have these partial results.

Copy link
Contributor Author

@Nigusu-Allehu Nigusu-Allehu Nov 14, 2023

Choose a reason for hiding this comment

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

Currently the way I have it implemented is so that if a search times out for one source, we print an error, skip the source, and we continue to the next source. The only situation where we exit with 1 is if the command is badly formatted or an invalid source is specified. If the command can be parsed and the source identified, it will always exit with 0. However, if anything goes wrong for a specific source search, the appropriate error is printed out and the search continues to the next source.

From my understanding, nuget.exe list on the other hand does not catch such exceptions, it just lets them be thrown. The nuget.exe program is designed so that if any command throws an exception, it exits with 1 and prints the error. Therefore a nuget.exe list command in the situation a source times out, will exit 1 and print the exception.

I could also have dotnet package search exit with 1 rather than continuing to search. I think I should make it part of the discussion in the "additional options" spec review.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok if we complete the operations for all sources and end up with an exit code for 1.
A single source failing would change the result for other sources, unlike say restore for example, where a single source being inaccessible could actually change what gets resolved.

Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Looks good! I left some nits and opinions which you can address now, later, or probably just ignore.

@Nigusu-Allehu Nigusu-Allehu merged commit eff2a58 into dev-feature-migrate-systemcommandline Nov 15, 2023
16 checks passed
@Nigusu-Allehu Nigusu-Allehu deleted the dev-nyenework-dotnet-package-search branch November 15, 2023 05:21
Nigusu-Allehu added a commit that referenced this pull request Nov 22, 2023
@martinrrm martinrrm mentioned this pull request Jan 9, 2024
8 tasks
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.

Support for dotnet search command (equivalent to nuget.exe list, later search)
6 participants