Show help only for the error itself has to do with a command, argument, or option being invalid with dotnet nuget command by erdembayar · Pull Request #4211 · NuGet/NuGet.Client

erdembayar · 2021-08-20T01:10:22Z

Bug

Fixes: NuGet/Home#10788

Regression? Last working version:

Description

Implemented sub commands so it show subcommand help instead of main `dotnet nuget trust` command help for all cases.

Previously, all options some not related to sub-command is displayed:

After, only relevant info for subcommand displayed.

Even main command help is changed:

Previously, doesn't show sub-commands.

After, sub commands are listed below:

Duplicate entry error(when try to add same trust certificate multiple times), doesn't display help info anymore:

Before:

After:

PR Checklist

src/NuGet.Core/NuGet.Commands/StringFormatter.cs

nkolev92 · 2021-08-27T02:13:46Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

  <data name="SignCommandNoCertificateException" xml:space="preserve">
    <value>No certificate was provided.</value>
  </data>
+  <data name="TrustAuthorCommandDescription" xml:space="preserve">


Have these strings been reviewed by a PM?

@chgill-MSFT

Chris already approved it. Could you be able to review again?

I don't really have any other concerns here.
Not signing off, but you already have the necessary 1 approval.

src/NuGet.Core/NuGet.Commands/StringFormatter.cs

kartheekp-ms · 2021-08-27T17:16:38Z

Erick - I had a quick look at the code, looks good so far. I think we need a follow-up issue to register subcommands in dotnet/sdk repo also. dotnet/sdk#18061 (comment)

erdembayar · 2021-08-27T17:44:43Z

Erick - I had a quick look at the code, looks good so far. I think we need a follow-up issue to register subcommands in dotnet/sdk repo also. dotnet/sdk#18061 (comment)

Sure, created follow up issue. dotnet/sdk#20249

src/NuGet.Clients/NuGet.PackageManagement.UI/NuGet.PackageManagement.UI.csproj

nkolev92

I'm happy to approve, I just want to make sure the user facing strings have been validated.

erdembayar · 2021-08-30T19:36:47Z

@chgill-MSFT
Please validate newly added strings here.

chrisraygill

Overall LGTM, a couple minor questions and one word suggestion.

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

src/NuGet.Core/NuGet.Commands/Strings.resx

erdembayar · 2021-09-01T23:05:18Z

I'm happy to approve, I just want to make sure the user facing strings have been validated.

I just addressed comments by Chris.

ghost · 2021-09-15T07:00:41Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

ghost · 2021-09-28T16:00:37Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

kartheekp-ms · 2021-10-02T02:22:35Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

+                    CommandOption owners = sourceCommand.Option(
+                        "--owners",
+                        Strings.TrustCommandOwners,
+                        CommandOptionType.MultipleValue);


As per doc, --owners option accepts list of values separated by semi-colon.

Copied from examples section in the doc, dotnet nuget trust source NuGetTrust https://api.nuget.org/v3/index.json --owners "Nuget;Microsoft"

I think CommandOptionType for this option should be SingleValue instead of MultipleValue. This comment applies to all the sub-commands which has owners as an option.

Ok. I addressed your concerns. Please review again.

erdembayar · 2021-10-04T23:10:35Z

@kartheekp-ms
If you don't have any other comment then I'll merge today evening. Nikolche already tacitly approved.

erdembayar · 2021-10-05T17:51:33Z

@kartheekp-ms If you don't have any other comment then I'll merge today evening. Nikolche already tacitly approved.

Actually I need another approval to merge. Please review.

kartheekp-ms · 2021-10-05T18:05:05Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

+
+                var runner = new TrustedSignersCommandRunner(trustedSignersProvider, sourceProvider);
+                Task<int> trustedSignTask = runner.ExecuteCommandAsync(trustedSignersArgs);
+                return await trustedSignTask;


Suggested change

return await trustedSignTask;

return trustedSignTask;

Even though this suggestion saves 1 machine state, but makes hard to track down aggregated exception(I couldn't find the link for ref, it brought up during last year learning day for async programming), maybe better to keep as it's since all other dotnet nuget implementation doing same.

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

kartheekp-ms · 2021-10-05T18:38:26Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

+                Task<int> trustedSignTask = runner.ExecuteCommandAsync(trustedSignersArgs);
+                return await trustedSignTask;
+            }
+            catch (InvalidOperationException e)


More I think about it, the correct place for exception handling would have been within in the subcommand.OnExecute code block because the handlers are specific to the sub-commands. It is possible that I may be missing something here.

I'll address this one next time I do NuGet/Home#10294

erdembayar added 8 commits August 17, 2021 21:23

Handle command exception internally.

4bca4ee

Revert unneeded change.

8ee7ea4

Refactor command to subcommands.

4312ac7

Refactor remaining nuget trust commands.

93e12d6

Duplicate certificate fingerprint is handled.

6b050db

Add missing option.

8bb6320

Clean up

f4e4111

Replace texts

07321fa

erdembayar requested a review from a team as a code owner August 20, 2021 01:10

erdembayar marked this pull request as draft August 20, 2021 01:10

Add tests.

9ce619b

erdembayar force-pushed the dev-eryondon-10788 branch from e00dfad to 9ce619b Compare August 20, 2021 01:24

Clean up

d87a925

erdembayar marked this pull request as ready for review August 20, 2021 01:27

nkolev92 reviewed Aug 27, 2021

View reviewed changes

Remove Stringformatter.

458fdee

erdembayar mentioned this pull request Aug 27, 2021

[Bug]: Add arity for dotnet nuget trust command NuGet/Home#11190

Closed

erdembayar mentioned this pull request Aug 27, 2021

Register subcommand for dotnet nuget trust command dotnet/sdk#20249

Closed

nkolev92 reviewed Aug 27, 2021

View reviewed changes

src/NuGet.Clients/NuGet.PackageManagement.UI/NuGet.PackageManagement.UI.csproj Outdated Show resolved Hide resolved

nkolev92 reviewed Aug 27, 2021

View reviewed changes

erdembayar requested a review from chrisraygill August 30, 2021 19:36

chrisraygill approved these changes Sep 1, 2021

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Commands/Strings.resx Outdated Show resolved Hide resolved

erdembayar force-pushed the dev-eryondon-10788 branch from d09a8bb to 608c510 Compare September 1, 2021 23:03

Address PR comments.

60efed9

erdembayar force-pushed the dev-eryondon-10788 branch from 608c510 to 60efed9 Compare September 1, 2021 23:04

erdembayar requested a review from nkolev92 September 2, 2021 16:33

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 15, 2021

Merge branch 'dev' into dev-eryondon-10788

49e0a25

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 21, 2021

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 28, 2021

erdembayar marked this pull request as draft September 29, 2021 17:56

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 29, 2021

Address PR comment

df62f89

erdembayar force-pushed the dev-eryondon-10788 branch from 3357cb3 to df62f89 Compare October 1, 2021 13:35

erdembayar marked this pull request as ready for review October 1, 2021 16:59

Clean up

ce8b719

erdembayar requested a review from kartheekp-ms October 1, 2021 18:38

kartheekp-ms reviewed Oct 2, 2021

View reviewed changes

erdembayar added 2 commits October 1, 2021 20:20

Address latest PR comment.

526b9ea

Merge branch 'dev' into dev-eryondon-10788

069b552

erdembayar force-pushed the dev-eryondon-10788 branch from f8ba841 to 069b552 Compare October 3, 2021 16:50

erdembayar requested a review from kartheekp-ms October 4, 2021 15:36

Skip flaky test

7f0e36f

erdembayar force-pushed the dev-eryondon-10788 branch from c5c7d7f to 7f0e36f Compare October 4, 2021 16:40

Add param

5db5ac7

kartheekp-ms approved these changes Oct 5, 2021

View reviewed changes

erdembayar merged commit 0c61455 into dev Oct 5, 2021

erdembayar deleted the dev-eryondon-10788 branch October 5, 2021 20:30

This was referenced Oct 5, 2021

Skip flaky test GetPackageForFSharpProjectReturnsCorrectPackages #4297

Closed

Disable flaky test GetPackageForFSharpProjectReturnsCorrectPackages NuGet/Home#11290

Closed

erdembayar mentioned this pull request Feb 10, 2022

Create NuGet 6.1 release notes NuGet/docs.microsoft.com-nuget#2667

Merged

Conversation

erdembayar commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

Implemented sub commands so it show subcommand help instead of main dotnet nuget trust command help for all cases.

Even main command help is changed:

Duplicate entry error(when try to add same trust certificate multiple times), doesn't display help info anymore:

PR Checklist

Uh oh!

Uh oh!

nkolev92 Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

erdembayar Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

nkolev92 Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kartheekp-ms commented Aug 27, 2021

Uh oh!

erdembayar commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

erdembayar commented Aug 30, 2021

Uh oh!

chrisraygill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erdembayar commented Sep 1, 2021

Uh oh!

ghost commented Sep 15, 2021

Uh oh!

ghost commented Sep 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kartheekp-ms Oct 2, 2021

Choose a reason for hiding this comment

Uh oh!

erdembayar Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erdembayar commented Oct 4, 2021

Uh oh!

erdembayar commented Oct 5, 2021

Uh oh!

kartheekp-ms Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

erdembayar Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kartheekp-ms Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

erdembayar Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

erdembayar commented Aug 20, 2021 •

edited

Loading

Implemented sub commands so it show subcommand help instead of main `dotnet nuget trust` command help for all cases.

erdembayar commented Aug 27, 2021 •

edited

Loading

erdembayar Oct 4, 2021 •

edited

Loading

erdembayar Oct 5, 2021 •

edited

Loading