LegalFilePathsOnly() should not allow empty string

[therealchjones]

command-line-api/src/System.CommandLine/ArgumentExtensions.cs

Line 179 in 2b7d982

public static TArgument LegalFileNamesOnly<TArgument>(

The argument validator LegalFilePathsOnly() (and the associated option extension) checks for disallowed characters such as path separators in the argument. However, empty strings are not caught by this validator, and when used for a FileInfo, FileSystemInfo, or DirectoryInfo option, the empty argument throws an exception. It would seem reasonable that rather than add a custom validator or parser, LegalFilePathsOnly() would create an error with such an argument.

Happy to work on PR if all agree.

[jonsequitur]

Could you provide an example of how the empty string gets passed and what error you're seeing?

[therealchjones]

using System;
using System.CommandLine;
using System.IO;

namespace command_line_api_test
{
	class Program
	{
		static void Main(string[] args)
		{
			RootCommand command = new RootCommand();
			Option<DirectoryInfo> dirOption =
				new Option<DirectoryInfo>("--dir", "directory")
				{
					IsRequired = true,
					Arity = ArgumentArity.ExactlyOne
				};
			dirOption.LegalFilePathsOnly();
			command.AddOption(dirOption);
			command.SetHandler((DirectoryInfo directory) =>
				Console.WriteLine($"Selected directory {directory}"), dirOption);
			command.Invoke(args);
			Console.WriteLine("Hello World!");
		}
	}
}

$ dotnet run -- --dir foo
Selected directory foo
Hello World!
$ dotnet run -- --dir ''
Unhandled exception. System.ArgumentException: The path is empty. (Parameter 'path')
   at System.IO.Path.GetFullPath(String path)
   at System.IO.DirectoryInfo..ctor(String path)
   at System.CommandLine.Binding.ArgumentConverter.<>c.<.cctor>b__16_4(String path, Object& value)
   at System.CommandLine.Binding.ArgumentConverter.<GetConverter>g__ConvertSingleString|3_2(ArgumentResult result, TryConvertString convert, Object& value)
   at System.CommandLine.Binding.ArgumentConverter.<>c__DisplayClass3_0.<GetConverter>b__1(ArgumentResult result, Object& value)
   at System.CommandLine.Parsing.ArgumentResult.Convert(Argument argument)
   at System.CommandLine.Parsing.ArgumentResult.GetArgumentConversionResult()
   at System.CommandLine.Parsing.ParseResultVisitor.ValidateAndConvertArgumentResult(ArgumentResult argumentResult)
   at System.CommandLine.Parsing.ParseResultVisitor.ValidateAndConvertOptionResult(OptionResult optionResult)
   at System.CommandLine.Parsing.ParseResultVisitor.Stop()
   at System.CommandLine.Parsing.ParseResultVisitor.Visit(SyntaxNode node)
   at System.CommandLine.Parsing.Parser.Parse(IReadOnlyList`1 arguments, String rawInput)
   at System.CommandLine.CommandExtensions.GetDefaultInvocationPipeline(Command command, String[] args)
   at System.CommandLine.CommandExtensions.Invoke(Command command, String[] args, IConsole console)
   at command_line_api_test.Program.Main(String[] args) in /Users/chjones/Downloads/command-line-api-test/Program.cs:line 22

[therealchjones]

Of course, since it won’t create the DirectoryInfo or FileInfo instance anyway (regardless of legal character validation), I suppose it might be more appropriate to fail with an error for all such options.

[jonsequitur]

I think it's probably fair to have a more explicit error message rather than letting the FileInfo constructor's exception try to communicate the issue. If you'd like to submit a PR, we'd appreciate it.

[therealchjones]

Will do!

…

On Apr 7, 2022, at 17:12, Jon Sequeira ***@***.***> wrote:  I think it's probably fair to have a more explicit error message rather than letting the FileInfo constructor's exception try to communicate the issue. If you'd like to submit a PR, we'd appreciate it. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.