`nuget sources`: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings #3112

rrelyea · 2019-11-06T01:07:20Z

Bug

Fixes: NuGet/Home#4126 (part 1 or 2 to support for dotnet nuget sources)
Regression: No

Fix

Details: This set of work has reorganized the code from a monolithic SourcesCommand in NuGet.CommandLine to a SourcesCommand which creates a SourcesArgs and passes it to a SourcesRunner. Args/Runner are in NuGet.Commands.
This should maintain current functionality in "NuGet Sources" while enabling the dotnet nuget sources work to be built on top of the Args/Runner in NuGet.Commands in another PR soon.

Testing/Validation

Tests Added: I fixed a test that was skipped and have it run now, but didn't see need to add any new tests at this stage. Let me know if I am wrong there.

Reason for not adding tests: not changing any functionality, fairly good coverage already.
Validation: ran unit tests

…witch

rrelyea · 2019-11-07T20:20:29Z

src/NuGet.Clients/NuGet.CommandLine/NuGetResources.resx

@@ -3047,474 +2990,6 @@ To prevent NuGet from downloading packages during build, open the Visual Studio
  <data name="InstallCommandPackageAlreadyExists_cht" xml:space="preserve">
    <value>已安裝封裝 "{0}"。</value>
  </data>
-  <data name="SourcesCommandNoSources_csy" xml:space="preserve">


Need to think through the localization impact of this string table change.
Moved the english strings into nuget.commands.dll, but deleted the localized strings in nuget.exe -- and was hoping standard localization would work. But NuGet.exe may be a different beast for loc.

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs

nkolev92

Some changes needed, mostly cleanup right now.

src/NuGet.Clients/NuGet.CommandLine/Commands/SourcesCommand.cs

src/NuGet.Clients/NuGet.CommandLine/Common/Verbosity.cs

src/NuGet.Core/NuGet.Commands/NuGet.CommandLine/SourcesListFormat.cs

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs

…cky' feedback

src/NuGet.Clients/NuGet.CommandLine/Commands/SourcesCommand.cs

src/NuGet.Clients/NuGet.CommandLine/Common/Verbosity.cs

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesArgs.cs

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs

donnie-msft

I like the improvements in the code, but want to make sure the throws becoming LogErrors is as expected. Assuming the spirit of this change is to show more validation errors to the user at once, rather than making them fix missing arguments 1 by 1.

src/NuGet.Clients/NuGet.CommandLine/Commands/SourcesCommand.cs

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesArgs.cs

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs

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

rrelyea · 2019-11-08T22:00:19Z

src/NuGet.Clients/NuGet.CommandLine/NuGetCommand.resx

@@ -5550,4 +5550,7 @@ nuget trusted-signers Remove -Name TrustedRepo</value>
  <data name="PackageCommandDeterministic" xml:space="preserve">
    <value>Specify if the command should create a deterministic package. Multiple invocations of the pack command will create the exact same package.</value>
  </data>
+  <data name="SourcesActionInvalid" xml:space="preserve">
+    <value>nuget sources</value>


this was likely a placeholder.

donnie-msft

Looks like we're back to a throw-on-error model, so looks good.

nkolev92

Looks good overall.

Some concern with the exception move. It feels like a leak.

nkolev92 · 2019-11-09T00:13:14Z

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs

+        {
+            if (string.IsNullOrEmpty(args.Name))
+            {
+                throw new CommandLineException(Strings.SourcesCommandNameRequired);


Why not use ArgumentException?

Discussed offline.

Whether we call this CommandLineException or something smarter like LocalizedException can be decided in the dotnet.exe side implementation of this.

fwiw, looking into our code...there's nothing that relies on this being a commandlineexception.

…eeds

rrelyea · 2020-02-12T21:48:47Z

STarted another PR to covert this work. Got checked into 5.5 recently. Closing this PR.

Rob Relyea added 6 commits November 5, 2019 17:04

sources: refactor into SourcesArgs/Runner and move strings around

8195cef

revert test back to skip

652e790

added verbosity support & properly lowercased the action before the s…

609644e

…witch

didn't mean to skip this test

831c11c

tweaks

fd55f84

move to use enums, instead of string compare

eeb7f07

rrelyea requested review from donnie-msft, heng-liu, kartheekp-ms, zivkan, zkat, nkolev92, dtivel and anangaur November 7, 2019 20:09

rrelyea marked this pull request as ready for review November 7, 2019 20:09

rrelyea requested a review from dominoFire as a code owner November 7, 2019 20:09

rrelyea changed the title ~~sources: refactor into SourcesArgs/Runner and move strings around~~ nuget sources: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings Nov 7, 2019

rrelyea commented Nov 7, 2019

View reviewed changes

loic-sharma reviewed Nov 7, 2019

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs Outdated Show resolved Hide resolved

removed extraneous SourcesListFormat enum

c75a540

nkolev92 reviewed Nov 7, 2019

View reviewed changes

addressing 'nuget.commandline is not a library' and 'verbosity is tri…

bf0b65c

…cky' feedback

nkolev92 reviewed Nov 8, 2019

View reviewed changes

src/NuGet.Clients/NuGet.CommandLine/Commands/SourcesCommand.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.CommandLine/Common/Verbosity.cs Outdated Show resolved Hide resolved

rrelyea commented Nov 8, 2019

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesArgs.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Nov 8, 2019

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Nov 8, 2019

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Nov 8, 2019

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourcesRunner.cs Outdated Show resolved Hide resolved

donnie-msft requested changes Nov 8, 2019

View reviewed changes

move errors back to exceptions, other tweaks from code review

7517978

rrelyea requested a review from donnie-msft November 8, 2019 21:50

rrelyea commented Nov 8, 2019

View reviewed changes

donnie-msft approved these changes Nov 8, 2019

View reviewed changes

nkolev92 approved these changes Nov 9, 2019

View reviewed changes

Rob Relyea added 2 commits November 8, 2019 17:34

CommandLineException -> CommandException

3b4a580

refactor sourcesArgs to allow logMinimalOverride - which dotnet.exe n…

527b030

…eeds

zivkan mentioned this pull request Jan 23, 2020

ClientsCertificate feature spec NuGet/Home#8823

Merged

rrelyea closed this Feb 12, 2020

rrelyea deleted the dev-rrelyea-sourcesStepByStep branch February 21, 2020 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`nuget sources`: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings #3112

`nuget sources`: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings #3112

rrelyea commented Nov 6, 2019 •

edited

Loading

rrelyea Nov 7, 2019 •

edited

Loading

nkolev92 left a comment

donnie-msft left a comment

rrelyea Nov 8, 2019

donnie-msft left a comment

nkolev92 left a comment

nkolev92 Nov 9, 2019

nkolev92 Nov 9, 2019

nkolev92 Nov 9, 2019

rrelyea commented Feb 12, 2020

nuget sources: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings #3112

nuget sources: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings #3112

Conversation

rrelyea commented Nov 6, 2019 • edited Loading

Bug

Fix

Testing/Validation

rrelyea Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

rrelyea Nov 8, 2019

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Nov 9, 2019

Choose a reason for hiding this comment

nkolev92 Nov 9, 2019

Choose a reason for hiding this comment

nkolev92 Nov 9, 2019

Choose a reason for hiding this comment

rrelyea commented Feb 12, 2020

`nuget sources`: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings #3112

`nuget sources`: move implementation into NuGet.Commands.dll - refactor into SourcesArgs/SourcesRunner - move enums/strings #3112

rrelyea commented Nov 6, 2019 •

edited

Loading

rrelyea Nov 7, 2019 •

edited

Loading