commandline icon indicating copy to clipboard operation
commandline copied to clipboard

Example with Option with IEnumerable<string> broken - Sequence contains more than one element

Open ivberg-zz opened this issue 6 years ago • 6 comments

Running the latest commandlineparser v2.3.0 on .Net Core 2.1.

The library example doesn't seem to work at all parsing IEnumerable [Option('r', "read", Required = true, HelpText = "Input files to be processed.")] public IEnumerable InputFiles { get; set; }

If you specify --read file1 file2 you get this exception:

System.InvalidOperationException HResult=0x80131509 Message=Sequence contains more than one element Source=System.Linq StackTrace:

System.Linq.dll!System.Linq.Enumerable.Single<System.Type>(System.Collections.Generic.IEnumerable<System.Type> source) Line 45 C# CommandLine.dll!CommandLine.Core.InstanceBuilder.Build.AnonymousMethod__0_23(CommandLine.Core.SpecificationProperty sp) Unknown CommandLine.dll!CommandLine.Core.ReflectionExtensions.SetProperties.AnonymousMethod__0(System.__Canon current, CommandLine.Core.SpecificationProperty specProp) Unknown System.Linq.dll!System.Linq.Enumerable.Aggregate<CommandLine.Core.SpecificationProperty, LogMerger.Program.Options>(System.Collections.Generic.IEnumerable<CommandLine.Core.SpecificationProperty> source, LogMerger.Program.Options seed, System.Func<LogMerger.Program.Options, CommandLine.Core.SpecificationProperty, LogMerger.Program.Options> func) Line 55 C# CommandLine.dll!CommandLine.Core.InstanceBuilder.Build.AnonymousMethod__16() Unknown CommandLine.dll!CommandLine.Core.InstanceBuilder.Build.AnonymousMethod__7() Unknown CommandLine.dll!CommandLine.Parser.ParseArguments<LogMerger.Program.Options>(System.Collections.Generic.IEnumerable args) Unknown

ivberg-zz avatar Dec 07 '18 00:12 ivberg-zz

Actually, this was an issue instead with Parsing a Dict public Dictionary<string, string> Dict { get; set; }

Is this supported?

ivberg-zz avatar Dec 07 '18 01:12 ivberg-zz

No, unfortunately dicts are not supported

nemec avatar Dec 07 '18 02:12 nemec

A dictionary would be hard to support... what generates the keys for the values?

I would believe this is undefined in the getopt specs that this library strives to follow.

ericnewton76 avatar Dec 12 '18 03:12 ericnewton76

I think you are correct Eric that it would be hard to support. I think this is more about protecting against dumb new user and a clearer exception behavior than implementing support :). As a C# dev, Dictionary is popular and for better or worse, it's what I expected/hoped to use here as one of the cmd line options. The above is actually misleading, as I thought the IEnumerable is what was causing the issue, but instead the Dict used as another possible cmd-line parameter (turns out didn't need it)

My suggestion is the user adds odd or not supported stuff, that we have a clearer exception than System.InvalidOperationException - Message=Sequence contains more than one element

ivberg-zz avatar Dec 12 '18 18:12 ivberg-zz

Indeed. The error is misleading.

ericnewton76 avatar Jan 23 '19 23:01 ericnewton76

Hi everyone,

I see that this issue has been open for a few years now, I'd like to make a recap and see this issue closed.

First of all, I noticed that the example code reported in the first message contains an error because IEnumerable should be IEnumerable<string> instead.

[Option('r', "read", Required = true, HelpText = "Input files to be processed.")]
public IEnumerable InputFiles { get; set; }

If we try to parse the following options, the library correctly throws a System.InvalidOperationException exception with a nice error message Non scalar properties should be sequence of type IEnumerable<T>..

internal class Options_IEnumerable_Without_Type
{
    [Option('r', "read", Required = true, HelpText = "Input files to be processed.")]
    public IEnumerable InputFiles { get; set; }
}

As @ivberg-zz and @eric pointed out later, this issue is actually about improving the error message when the developed tries to use an unsupported type (i.e. a dictionary).

I noticed that the user @phaniva made a pull request phaniva:issue-377 with a proposed solution of adding a new specification guard that ensures that all sequence type require exactly one argument.

private static Func<Specification, bool> GaurdAgainstUnsupportedSequenceTypes()
{
    return spec => spec.TargetType == TargetType.Sequence && spec.ConversionType.GetGenericArguments().Length != 1;
}

The problem with this solution is that the error Non scalar properties should be sequence of type IEnumerable<T>. defined in TypeConverter.ChangeTypeSequence will be bypassed - that means that developers will receive a different error message when trying to parse a IEnumerable without a specified type.

In my opinion there are two possible solutions to the problem:

  1. To ensure - inside ChangeTypeSequence - that the sequence has exactly one generic argument.
  2. To add two new rules inside SpecificationGuards that ensure that sequence types have exactly one generic argument.

Is it fair to have some checks inside the ChangeTypeSequence and some checks inside SpecificationGuards? Shouldn't we maybe try to move all checks to SpecificationGuards?

Thanks in advance and kind regards, Davide

PentaPinguino avatar Jul 23 '23 13:07 PentaPinguino