dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.68k stars 1.06k forks source link

`dotnet nuget sources add` or `dotnet nuget add source` ? #29480

Open dominoFire opened 1 year ago

dominoFire commented 1 year ago

While working on System.CommandLine migration, I discovered that we have two commands that refer to the same action:

Question: which command should we preserve in dotnet.exe NuGet commands?

  1. dotnet nuget sources add
  2. dotnet nuget add source
  3. Both

Running dotnet nuget sources add gives:

PS C:\> dotnet nuget sources add
Specify --help for a list of available options and commands.
log  : The proper command is 'dotnet nuget add source'.

Running dotnet nuget add source

PS C:\> dotnet nuget add source
error: System.ArgumentNullException: Value cannot be null. (Parameter 'input')
error:    at System.Text.RegularExpressions.ThrowHelper.ThrowArgumentNullException(ExceptionArgument arg)
error:    at System.Text.RegularExpressions.Regex.IsMatch(String input)
error:    at NuGet.Common.PathValidator.IsValidUrl(String url)
error:    at NuGet.Common.PathValidator.IsValidSource(String source)
error:    at NuGet.Commands.AddSourceRunner.Run(AddSourceArgs args, Func`1 getLogger)
error:    at NuGet.CommandLine.XPlat.AddVerbParser.<>c__DisplayClass0_1.<Register>b__4()
error:    at Microsoft.Extensions.CommandLineUtils.CommandLineApplication.Execute(String[] args)
error:    at NuGet.CommandLine.XPlat.Program.MainInternal(String[] args, CommandOutputLogger log)

Usage: dotnet nuget add source [arguments] [options]

Arguments:
  PackageSourcePath  Path to the package source.

Options:
  -n|--name                       Name of the source.
  -u|--username                   Username to be used when connecting to an authenticated source.
  -p|--password                   Password to be used when connecting to an authenticated source.
  --store-password-in-clear-text  Enables storing portable package source credentials by disabling password encryption.
  --valid-authentication-types    Comma-separated list of valid authentication types for this source. Set this to basic if the server advertises NTLM or Negotiate and your credentials must be sent using the Basic mechanism, for instance when using a PAT with on-premises Azure DevOps Server. Other valid values include negotiate, kerberos, ntlm, and digest, but these values are unlikely to be useful.
  --configfile                    The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used. For more information, see https://docs.microsoft.com/nuget/consume-packages/configuring-nuget-behavior.
  -h|--help                       Show help information

cc @baronfel

baronfel commented 1 year ago

Two commands isn't unheard of - docker for example has both noun-first and verb-first versions, and often aliases them both to the same underlying command handler. In the .NET CLI for recent versions, we try to prefer noun-first where possible, but the nuget command already doesn't align with this (nor does the add command on the CLI itself, which is a bit older). So there's not a 100 % consistent guideline that's applicable here.

I would say that consistency within the dotnet nuget command is the first priority, but it should be very easy to support both forms in System.CommandLine if you wanted to reach a bit. The noun-first command could even emitt a message with the correct format. The dotnet new command did this for .NET 7 for deprecated forms of their own list, search, etc commands:

15:28:15 ❯ dotnet new --list blah
Warning: use of 'dotnet new --list' is deprecated. Use 'dotnet new list' instead.
For more information, run:
   dotnet new list -h

No templates found matching: 'blah'.

To search for the templates on NuGet.org, run:
   dotnet new search blah

For details on the exit code, refer to https://aka.ms/templating-exit-codes#103
dominoFire commented 1 year ago

Per spec, dotnet add source was preferred to be consistent with .NET CLI in version 1.x https://github.com/NuGet/Home/wiki/Add-nuget-sources-command-to-the-dotnet-CLI

Then, I will try to align to .NET 7 noun-first approach: