From 78171b080e04e1054a19ed2c72de05635c07ebc9 Mon Sep 17 00:00:00 2001 From: Tyler Dunkel <40210514+tydunkel@users.noreply.github.com> Date: Sun, 26 May 2019 21:32:44 -0700 Subject: [PATCH 1/3] Add multi-instance option support --- src/CommandLine/Core/InstanceBuilder.cs | 31 +++- src/CommandLine/Core/InstanceChooser.cs | 29 +++- src/CommandLine/Core/OptionMapper.cs | 34 ++-- src/CommandLine/Core/Sequence.cs | 153 +++++++++++++++--- .../Core/SpecificationPropertyRules.cs | 17 +- src/CommandLine/Core/TokenPartitioner.cs | 5 +- src/CommandLine/Core/TypeConverter.cs | 2 +- src/CommandLine/Parser.cs | 5 +- src/CommandLine/ParserSettings.cs | 10 ++ .../Unit/Core/InstanceBuilderTests.cs | 14 +- .../Unit/Core/InstanceChooserTests.cs | 17 +- .../Unit/Core/OptionMapperTests.cs | 62 +++++++ .../Unit/Core/SequenceTests.cs | 64 +++++++- .../Core/SpecificationPropertyRulesTests.cs | 58 +++++++ .../Unit/Core/TypeConverterTests.cs | 10 ++ tests/CommandLine.Tests/Unit/ParserTests.cs | 66 +++++--- 16 files changed, 505 insertions(+), 72 deletions(-) create mode 100644 tests/CommandLine.Tests/Unit/Core/SpecificationPropertyRulesTests.cs diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs index dce377f1..788ce187 100644 --- a/src/CommandLine/Core/InstanceBuilder.cs +++ b/src/CommandLine/Core/InstanceBuilder.cs @@ -23,6 +23,31 @@ public static ParserResult Build( bool autoHelp, bool autoVersion, IEnumerable nonFatalErrors) + { + return Build( + factory, + tokenizer, + arguments, + nameComparer, + ignoreValueCase, + parsingCulture, + autoHelp, + autoVersion, + false, + nonFatalErrors); + } + + public static ParserResult Build( + Maybe> factory, + Func, IEnumerable, Result, Error>> tokenizer, + IEnumerable arguments, + StringComparer nameComparer, + bool ignoreValueCase, + CultureInfo parsingCulture, + bool autoHelp, + bool autoVersion, + bool allowMultiInstance, + IEnumerable nonFatalErrors) { var typeInfo = factory.MapValueOrDefault(f => f().GetType(), typeof(T)); @@ -70,7 +95,7 @@ public static ParserResult Build( var valueSpecPropsResult = ValueMapper.MapValues( (from pt in specProps where pt.Specification.IsValue() orderby ((ValueSpecification)pt.Specification).Index select pt), - valuesPartition, + valuesPartition, (vals, type, isScalar) => TypeConverter.ChangeType(vals, type, isScalar, parsingCulture, ignoreValueCase)); var missingValueErrors = from token in errorsPartition @@ -86,7 +111,7 @@ public static ParserResult Build( //build the instance, determining if the type is mutable or not. T instance; - if(typeInfo.IsMutable() == true) + if (typeInfo.IsMutable() == true) { instance = BuildMutable(factory, specPropsWithValue, setPropertyErrors); } @@ -95,7 +120,7 @@ public static ParserResult Build( instance = BuildImmutable(typeInfo, factory, specProps, specPropsWithValue, setPropertyErrors); } - var validationErrors = specPropsWithValue.Validate(SpecificationPropertyRules.Lookup(tokens)); + var validationErrors = specPropsWithValue.Validate(SpecificationPropertyRules.Lookup(tokens, allowMultiInstance)); var allErrors = tokenizerResult.SuccessMessages() diff --git a/src/CommandLine/Core/InstanceChooser.cs b/src/CommandLine/Core/InstanceChooser.cs index f3ab9b99..2b868f7c 100644 --- a/src/CommandLine/Core/InstanceChooser.cs +++ b/src/CommandLine/Core/InstanceChooser.cs @@ -22,6 +22,31 @@ public static ParserResult Choose( bool autoHelp, bool autoVersion, IEnumerable nonFatalErrors) + { + return Choose( + tokenizer, + types, + arguments, + nameComparer, + ignoreValueCase, + parsingCulture, + autoHelp, + autoVersion, + false, + nonFatalErrors); + } + + public static ParserResult Choose( + Func, IEnumerable, Result, Error>> tokenizer, + IEnumerable types, + IEnumerable arguments, + StringComparer nameComparer, + bool ignoreValueCase, + CultureInfo parsingCulture, + bool autoHelp, + bool autoVersion, + bool allowMultiInstance, + IEnumerable nonFatalErrors) { var verbs = Verb.SelectFromTypes(types); var defaultVerbs = verbs.Where(t => t.Item1.IsDefault); @@ -46,7 +71,7 @@ public static ParserResult Choose( arguments.Skip(1).FirstOrDefault() ?? string.Empty, nameComparer)) : (autoVersion && preprocCompare("version")) ? MakeNotParsed(types, new VersionRequestedError()) - : MatchVerb(tokenizer, verbs, defaultVerb, arguments, nameComparer, ignoreValueCase, parsingCulture, autoHelp, autoVersion, nonFatalErrors); + : MatchVerb(tokenizer, verbs, defaultVerb, arguments, nameComparer, ignoreValueCase, parsingCulture, autoHelp, autoVersion, allowMultiInstance, nonFatalErrors); }; return arguments.Any() @@ -92,6 +117,7 @@ private static ParserResult MatchVerb( CultureInfo parsingCulture, bool autoHelp, bool autoVersion, + bool allowMultiInstance, IEnumerable nonFatalErrors) { return verbs.Any(a => nameComparer.Equals(a.Item1.Name, arguments.First())) @@ -106,6 +132,7 @@ private static ParserResult MatchVerb( parsingCulture, autoHelp, autoVersion, + allowMultiInstance, nonFatalErrors) : MatchDefaultVerb(tokenizer, verbs, defaultVerb, arguments, nameComparer, ignoreValueCase, parsingCulture, autoHelp, autoVersion, nonFatalErrors); } diff --git a/src/CommandLine/Core/OptionMapper.cs b/src/CommandLine/Core/OptionMapper.cs index 18349b40..f01f14ee 100644 --- a/src/CommandLine/Core/OptionMapper.cs +++ b/src/CommandLine/Core/OptionMapper.cs @@ -22,26 +22,32 @@ public static Result< .Select( pt => { - var matched = options.FirstOrDefault(s => + var matched = options.Where(s => s.Key.MatchName(((OptionSpecification)pt.Specification).ShortName, ((OptionSpecification)pt.Specification).LongName, comparer)).ToMaybe(); - return matched.IsJust() - ? ( - from sequence in matched - from converted in - converter( - sequence.Value, - pt.Property.PropertyType, - pt.Specification.TargetType != TargetType.Sequence) - select Tuple.Create( - pt.WithValue(Maybe.Just(converted)), Maybe.Nothing()) - ) + + if (matched.IsJust()) + { + var matches = matched.GetValueOrDefault(Enumerable.Empty>>()); + var values = new HashSet(); + foreach (var kvp in matches) + { + foreach (var value in kvp.Value) + { + values.Add(value); + } + } + + return converter(values, pt.Property.PropertyType, pt.Specification.TargetType != TargetType.Sequence) + .Select(value => Tuple.Create(pt.WithValue(Maybe.Just(value)), Maybe.Nothing())) .GetValueOrDefault( Tuple.Create>( pt, Maybe.Just( new BadFormatConversionError( - ((OptionSpecification)pt.Specification).FromOptionSpecification())))) - : Tuple.Create(pt, Maybe.Nothing()); + ((OptionSpecification)pt.Specification).FromOptionSpecification())))); + } + + return Tuple.Create(pt, Maybe.Nothing()); } ).Memoize(); return Result.Succeed( diff --git a/src/CommandLine/Core/Sequence.cs b/src/CommandLine/Core/Sequence.cs index 04d1b4ae..10b9c600 100644 --- a/src/CommandLine/Core/Sequence.cs +++ b/src/CommandLine/Core/Sequence.cs @@ -14,30 +14,141 @@ public static IEnumerable Partition( IEnumerable tokens, Func> typeLookup) { - return from tseq in tokens.Pairwise( - (f, s) => - f.IsName() && s.IsValue() - ? typeLookup(f.Text).MapValueOrDefault(info => - info.TargetType == TargetType.Sequence - ? new[] { f }.Concat(tokens.OfSequence(f, info)) - : new Token[] { }, new Token[] { }) - : new Token[] { }) - from t in tseq - select t; - } + var sequences = new Dictionary>(); + var state = SequenceState.TokenSearch; + Token nameToken = default; + foreach (var token in tokens) + { + switch (state) + { + case SequenceState.TokenSearch: + if (token.IsName()) + { + if (typeLookup(token.Text).MatchJust(out var info) && info.TargetType == TargetType.Sequence) + { + nameToken = token; + state = SequenceState.TokenFound; + } + } + break; - private static IEnumerable OfSequence(this IEnumerable tokens, Token nameToken, TypeDescriptor info) - { - var nameIndex = tokens.IndexOf(t => t.Equals(nameToken)); - if (nameIndex >= 0) + case SequenceState.TokenFound: + if (token.IsValue()) + { + if (sequences.TryGetValue(nameToken, out var sequence)) + { + sequence.Add(token); + } + else + { + sequences[nameToken] = new List(new[] { token }); + } + } + else if (token.IsName()) + { + if (typeLookup(token.Text).MatchJust(out var info) && info.TargetType == TargetType.Sequence) + { + nameToken = token; + state = SequenceState.TokenFound; + } + else + { + state = SequenceState.TokenSearch; + } + } + else + { + state = SequenceState.TokenSearch; + } + break; + } + } + + foreach (var kvp in sequences) { - return info.NextValue.MapValueOrDefault( - _ => info.MaxItems.MapValueOrDefault( - n => tokens.Skip(nameIndex + 1).Take(n), - tokens.Skip(nameIndex + 1).TakeWhile(v => v.IsValue())), - tokens.Skip(nameIndex + 1).TakeWhile(v => v.IsValue())); + yield return kvp.Key; + foreach (var value in kvp.Value) + { + yield return value; + } } - return new Token[] { }; + + //return from tseq in tokens.Pairwise( + //(f, s) => + // f.IsName() && s.IsValue() + // ? typeLookup(f.Text).MapValueOrDefault(info => + // info.TargetType == TargetType.Sequence + // ? new[] { f }.Concat(tokens.OfSequence(f, info)) + // : new Token[] { }, new Token[] { }) + // : new Token[] { }) + // from t in tseq + // select t; + } + + //private static IEnumerable OfSequence(this IEnumerable tokens, Token nameToken, TypeDescriptor info) + //{ + // var state = SequenceState.TokenSearch; + // var count = 0; + // var max = info.MaxItems.GetValueOrDefault(int.MaxValue); + // var values = max != int.MaxValue + // ? new List(max) + // : new List(); + + // foreach (var token in tokens) + // { + // if (count == max) + // { + // break; + // } + + // switch (state) + // { + // case SequenceState.TokenSearch: + // if (token.IsName() && token.Text.Equals(nameToken.Text)) + // { + // state = SequenceState.TokenFound; + // } + // break; + + // case SequenceState.TokenFound: + // if (token.IsValue()) + // { + // state = SequenceState.ValueFound; + // count++; + // values.Add(token); + // } + // else + // { + // // Invalid to provide option without value + // return Enumerable.Empty(); + // } + // break; + + // case SequenceState.ValueFound: + // if (token.IsValue()) + // { + // count++; + // values.Add(token); + // } + // else if (token.IsName() && token.Text.Equals(nameToken.Text)) + // { + // state = SequenceState.TokenFound; + // } + // else + // { + // state = SequenceState.TokenSearch; + // } + // break; + // } + // } + + // return values; + //} + + private enum SequenceState + { + TokenSearch, + TokenFound, } } } diff --git a/src/CommandLine/Core/SpecificationPropertyRules.cs b/src/CommandLine/Core/SpecificationPropertyRules.cs index 5dc1a406..4f8b78a9 100644 --- a/src/CommandLine/Core/SpecificationPropertyRules.cs +++ b/src/CommandLine/Core/SpecificationPropertyRules.cs @@ -13,6 +13,14 @@ static class SpecificationPropertyRules public static IEnumerable, IEnumerable>> Lookup( IEnumerable tokens) + { + return Lookup(tokens, false); + } + + public static IEnumerable, IEnumerable>> + Lookup( + IEnumerable tokens, + bool allowMultiInstance) { return new List, IEnumerable>> { @@ -21,7 +29,7 @@ public static IEnumerable, IEnumerable, IEnumerable> EnforceSingle(IEnumerable tokens) + private static Func, IEnumerable> EnforceSingle(IEnumerable tokens, bool allowMultiInstance) { return specProps => { + if (allowMultiInstance) + { + return Enumerable.Empty(); + } + var specs = from sp in specProps where sp.Specification.IsOption() where sp.Value.IsJust() diff --git a/src/CommandLine/Core/TokenPartitioner.cs b/src/CommandLine/Core/TokenPartitioner.cs index be38a6d0..608ae0e8 100644 --- a/src/CommandLine/Core/TokenPartitioner.cs +++ b/src/CommandLine/Core/TokenPartitioner.cs @@ -21,10 +21,11 @@ Tuple>>, IEnumerable(Switch.Partition(tokenList, typeLookup), tokenComparer); var scalars = new HashSet(Scalar.Partition(tokenList, typeLookup), tokenComparer); var sequences = new HashSet(Sequence.Partition(tokenList, typeLookup), tokenComparer); + var dedupedSequences = new HashSet(sequences); var nonOptions = tokenList .Where(t => !switches.Contains(t)) .Where(t => !scalars.Contains(t)) - .Where(t => !sequences.Contains(t)).Memoize(); + .Where(t => !dedupedSequences.Contains(t)).Memoize(); var values = nonOptions.Where(v => v.IsValue()).Memoize(); var errors = nonOptions.Except(values, (IEqualityComparer)ReferenceEqualityComparer.Default).Memoize(); @@ -36,4 +37,4 @@ Tuple>>, IEnumerable ChangeType(IEnumerable values, Type conversionType, bool scalar, CultureInfo conversionCulture, bool ignoreValueCase) { return scalar - ? ChangeTypeScalar(values.Single(), conversionType, conversionCulture, ignoreValueCase) + ? ChangeTypeScalar(values.Last(), conversionType, conversionCulture, ignoreValueCase) : ChangeTypeSequence(values, conversionType, conversionCulture, ignoreValueCase); } diff --git a/src/CommandLine/Parser.cs b/src/CommandLine/Parser.cs index f801c0f7..10c9b4e1 100644 --- a/src/CommandLine/Parser.cs +++ b/src/CommandLine/Parser.cs @@ -101,6 +101,7 @@ public ParserResult ParseArguments(IEnumerable args) settings.ParsingCulture, settings.AutoHelp, settings.AutoVersion, + settings.AllowMultiInstance, HandleUnknownArguments(settings.IgnoreUnknownArguments)), settings); } @@ -131,6 +132,7 @@ public ParserResult ParseArguments(Func factory, IEnumerable ar settings.ParsingCulture, settings.AutoHelp, settings.AutoVersion, + settings.AllowMultiInstance, HandleUnknownArguments(settings.IgnoreUnknownArguments)), settings); } @@ -163,6 +165,7 @@ public ParserResult ParseArguments(IEnumerable args, params Type settings.ParsingCulture, settings.AutoHelp, settings.AutoVersion, + settings.AllowMultiInstance, HandleUnknownArguments(settings.IgnoreUnknownArguments)), settings); } @@ -228,4 +231,4 @@ private void Dispose(bool disposing) } } } -} \ No newline at end of file +} diff --git a/src/CommandLine/ParserSettings.cs b/src/CommandLine/ParserSettings.cs index 07c10c4c..95a4cd81 100644 --- a/src/CommandLine/ParserSettings.cs +++ b/src/CommandLine/ParserSettings.cs @@ -25,6 +25,7 @@ public class ParserSettings : IDisposable private CultureInfo parsingCulture; private bool enableDashDash; private int maximumDisplayWidth; + private bool allowMultiInstance; /// /// Initializes a new instance of the class. @@ -174,6 +175,15 @@ public int MaximumDisplayWidth set { maximumDisplayWidth = value; } } + /// + /// Gets or sets a value indicating whether options are allowed to be specified multiple times. + /// + public bool AllowMultiInstance + { + get => allowMultiInstance; + set => PopsicleSetter.Set(Consumed, ref allowMultiInstance, value); + } + internal StringComparer NameComparer { get diff --git a/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs b/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs index 8dd7371c..8b95af1c 100644 --- a/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs +++ b/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs @@ -19,7 +19,7 @@ namespace CommandLine.Tests.Unit.Core { public class InstanceBuilderTests { - private static ParserResult InvokeBuild(string[] arguments, bool autoHelp = true, bool autoVersion = true) + private static ParserResult InvokeBuild(string[] arguments, bool autoHelp = true, bool autoVersion = true, bool multiInstance = false) where T : new() { return InstanceBuilder.Build( @@ -31,6 +31,7 @@ private static ParserResult InvokeBuild(string[] arguments, bool autoHelp CultureInfo.InvariantCulture, autoHelp, autoVersion, + multiInstance, Enumerable.Empty()); } @@ -1235,6 +1236,17 @@ public void Options_In_Group_Do_Not_Allow_Mutually_Exclusive_Set() errors.Should().BeEquivalentTo(expectedResult); } + [Fact] + public void Parse_int_sequence_with_multi_instance() + { + var expected = new[] { 1, 2, 3 }; + var result = InvokeBuild( + new[] { "--int-seq", "1", "2", "--int-seq", "3" }, + multiInstance: true); + + ((Parsed)result).Value.IntSequence.Should().BeEquivalentTo(expected); + } + #region custom types diff --git a/tests/CommandLine.Tests/Unit/Core/InstanceChooserTests.cs b/tests/CommandLine.Tests/Unit/Core/InstanceChooserTests.cs index c9dae5fb..d5cb9a21 100644 --- a/tests/CommandLine.Tests/Unit/Core/InstanceChooserTests.cs +++ b/tests/CommandLine.Tests/Unit/Core/InstanceChooserTests.cs @@ -15,7 +15,8 @@ public class InstanceChooserTests { private static ParserResult InvokeChoose( IEnumerable types, - IEnumerable arguments) + IEnumerable arguments, + bool multiInstance = false) { return InstanceChooser.Choose( (args, optionSpecs) => Tokenizer.ConfigureTokenizer(StringComparer.Ordinal, false, false)(args, optionSpecs), @@ -26,6 +27,7 @@ private static ParserResult InvokeChoose( CultureInfo.InvariantCulture, true, true, + multiInstance, Enumerable.Empty()); } @@ -168,5 +170,18 @@ public void Parse_sequence_verb_with_separator_returns_verb_instance(string[] ar expected.Should().BeEquivalentTo(((Parsed)result).Value); // Teardown } + + [Fact] + public void Parse_sequence_verb_with_multi_instance_returns_verb_instance() + { + var expected = new SequenceOptions { LongSequence = new long[] { }, StringSequence = new[] { "s1", "s2" } }; + var result = InvokeChoose( + new[] { typeof(Add_Verb), typeof(Commit_Verb), typeof(Clone_Verb), typeof(SequenceOptions) }, + new[] { "sequence", "-s", "s1", "-s", "s2" }, + true); + + Assert.IsType(((Parsed)result).Value); + expected.Should().BeEquivalentTo(((Parsed)result).Value); + } } } diff --git a/tests/CommandLine.Tests/Unit/Core/OptionMapperTests.cs b/tests/CommandLine.Tests/Unit/Core/OptionMapperTests.cs index b2219683..63bf22f3 100644 --- a/tests/CommandLine.Tests/Unit/Core/OptionMapperTests.cs +++ b/tests/CommandLine.Tests/Unit/Core/OptionMapperTests.cs @@ -49,5 +49,67 @@ public void Map_boolean_switch_creates_boolean_value() // Teardown } + + [Fact] + public void Map_with_multi_instance_scalar() + { + var tokenPartitions = new[] + { + new KeyValuePair>("s", new[] { "string1" }), + new KeyValuePair>("shortandlong", new[] { "string2" }), + new KeyValuePair>("shortandlong", new[] { "string3" }), + new KeyValuePair>("s", new[] { "string4" }), + }; + + var specProps = new[] + { + SpecificationProperty.Create( + new OptionSpecification("s", "shortandlong", false, string.Empty, Maybe.Nothing(), Maybe.Nothing(), '\0', Maybe.Nothing(), string.Empty, string.Empty, new List(), typeof(string), TargetType.Scalar, string.Empty), + typeof(Simple_Options).GetProperties().Single(p => p.Name.Equals(nameof(Simple_Options.ShortAndLong), StringComparison.Ordinal)), + Maybe.Nothing()), + }; + + var result = OptionMapper.MapValues( + specProps.Where(pt => pt.Specification.IsOption()), + tokenPartitions, + (vals, type, isScalar) => TypeConverter.ChangeType(vals, type, isScalar, CultureInfo.InvariantCulture, false), + StringComparer.Ordinal); + + var property = result.SucceededWith().Single(); + Assert.True(property.Specification.IsOption()); + Assert.True(property.Value.MatchJust(out var stringVal)); + Assert.Equal(tokenPartitions.Last().Value.Last(), stringVal); + } + + [Fact] + public void Map_with_multi_instance_sequence() + { + var tokenPartitions = new[] + { + new KeyValuePair>("i", new [] { "1", "2" }), + new KeyValuePair>("i", new [] { "3" }), + new KeyValuePair>("i", new [] { "4", "5" }), + }; + var specProps = new[] + { + SpecificationProperty.Create( + new OptionSpecification("i", string.Empty, false, string.Empty, Maybe.Nothing(), Maybe.Nothing(), '\0', Maybe.Nothing(), string.Empty, string.Empty, new List(), typeof(IEnumerable), TargetType.Sequence, string.Empty), + typeof(Simple_Options).GetProperties().Single(p => p.Name.Equals(nameof(Simple_Options.IntSequence), StringComparison.Ordinal)), + Maybe.Nothing()) + }; + + var result = OptionMapper.MapValues( + specProps.Where(pt => pt.Specification.IsOption()), + tokenPartitions, + (vals, type, isScalar) => TypeConverter.ChangeType(vals, type, isScalar, CultureInfo.InvariantCulture, false), + StringComparer.Ordinal); + + var property = result.SucceededWith().Single(); + Assert.True(property.Specification.IsOption()); + Assert.True(property.Value.MatchJust(out var sequence)); + + var expected = tokenPartitions.Aggregate(Enumerable.Empty(), (prev, part) => prev.Concat(part.Value.Select(i => int.Parse(i)))); + Assert.Equal(expected, sequence); + } } } diff --git a/tests/CommandLine.Tests/Unit/Core/SequenceTests.cs b/tests/CommandLine.Tests/Unit/Core/SequenceTests.cs index b26575b8..65d3dd3e 100644 --- a/tests/CommandLine.Tests/Unit/Core/SequenceTests.cs +++ b/tests/CommandLine.Tests/Unit/Core/SequenceTests.cs @@ -49,7 +49,7 @@ public void Partition_sequence_values() } [Fact] - public void Partition_sequence_values_from_two_sequneces() + public void Partition_sequence_values_from_two_sequences() { var expected = new[] { @@ -93,5 +93,67 @@ public void Partition_sequence_values_only() expected.Should().BeEquivalentTo(result); } + + [Fact] + public void Partition_sequence_multi_instance() + { + var expected = new[] + { + Token.Name("seq"), + Token.Value("seqval0"), + Token.Value("seqval1"), + Token.Value("seqval2"), + Token.Value("seqval3"), + Token.Value("seqval4"), + }; + + var result = Sequence.Partition( + new[] + { + Token.Name("str"), Token.Value("strvalue"), Token.Value("freevalue"), + Token.Name("seq"), Token.Value("seqval0"), Token.Value("seqval1"), + Token.Name("x"), Token.Value("freevalue2"), + Token.Name("seq"), Token.Value("seqval2"), Token.Value("seqval3"), + Token.Name("seq"), Token.Value("seqval4") + }, + name => + new[] { "seq" }.Contains(name) + ? Maybe.Just(TypeDescriptor.Create(TargetType.Sequence, Maybe.Nothing())) + : Maybe.Nothing()); + + var actual = result.ToArray(); + Assert.Equal(expected, actual); + } + + [Fact] + public void Partition_sequence_multi_instance_with_max() + { + var expected = new[] + { + Token.Name("seq"), + Token.Value("seqval0"), + Token.Value("seqval1"), + Token.Value("seqval2"), + Token.Value("seqval3"), + Token.Value("seqval4"), + Token.Value("seqval5"), + }; + + var result = Sequence.Partition( + new[] + { + Token.Name("str"), Token.Value("strvalue"), Token.Value("freevalue"), + Token.Name("seq"), Token.Value("seqval0"), Token.Value("seqval1"), + Token.Name("x"), Token.Value("freevalue2"), + Token.Name("seq"), Token.Value("seqval2"), Token.Value("seqval3"), + Token.Name("seq"), Token.Value("seqval4"), Token.Value("seqval5"), + }, + name => + new[] { "seq" }.Contains(name) + ? Maybe.Just(TypeDescriptor.Create(TargetType.Sequence, Maybe.Just(3))) + : Maybe.Nothing()); + + Assert.Equal(expected, result); + } } } diff --git a/tests/CommandLine.Tests/Unit/Core/SpecificationPropertyRulesTests.cs b/tests/CommandLine.Tests/Unit/Core/SpecificationPropertyRulesTests.cs new file mode 100644 index 00000000..6e055c55 --- /dev/null +++ b/tests/CommandLine.Tests/Unit/Core/SpecificationPropertyRulesTests.cs @@ -0,0 +1,58 @@ +using CommandLine.Core; +using CommandLine.Tests.Fakes; +using CSharpx; +using System.Collections.Generic; +using Xunit; + +namespace CommandLine.Tests.Unit.Core +{ + + public class SpecificationPropertyRulesTests + { + [Fact] + public void Lookup_allows_multi_instance() + { + var tokens = new[] + { + Token.Name("name"), + Token.Value("value"), + Token.Name("name"), + Token.Value("value2"), + }; + + var specProps = new[] + { + SpecificationProperty.Create( + new OptionSpecification(string.Empty, "name", false, string.Empty, Maybe.Nothing(), Maybe.Nothing(), '\0', Maybe.Nothing(), string.Empty, string.Empty, new List(), typeof(IEnumerable), TargetType.Sequence, string.Empty), + typeof(SequenceOptions).GetProperty(nameof(SequenceOptions.StringSequence)), + Maybe.Just(new object())), + }; + + var results = specProps.Validate(SpecificationPropertyRules.Lookup(tokens, true)); + Assert.Empty(results); + } + + [Fact] + public void Lookup_fails_with_repeated_options_false_multi_instance() + { + var tokens = new[] + { + Token.Name("name"), + Token.Value("value"), + Token.Name("name"), + Token.Value("value2"), + }; + + var specProps = new[] + { + SpecificationProperty.Create( + new OptionSpecification(string.Empty, "name", false, string.Empty, Maybe.Nothing(), Maybe.Nothing(), '\0', Maybe.Nothing(), string.Empty, string.Empty, new List(), typeof(IEnumerable), TargetType.Sequence, string.Empty), + typeof(SequenceOptions).GetProperty(nameof(SequenceOptions.StringSequence)), + Maybe.Just(new object())), + }; + + var results = specProps.Validate(SpecificationPropertyRules.Lookup(tokens, false)); + Assert.Contains(results, r => r.GetType() == typeof(RepeatedOptionError)); + } + } +} diff --git a/tests/CommandLine.Tests/Unit/Core/TypeConverterTests.cs b/tests/CommandLine.Tests/Unit/Core/TypeConverterTests.cs index d9f3988c..ed9d6015 100644 --- a/tests/CommandLine.Tests/Unit/Core/TypeConverterTests.cs +++ b/tests/CommandLine.Tests/Unit/Core/TypeConverterTests.cs @@ -33,6 +33,16 @@ public void ChangeType_scalars(string testValue, Type destinationType, bool expe } } + [Fact] + public void ChangeType_Scalar_LastOneWins() + { + var values = new[] { "100", "200", "300", "400", "500" }; + var result = TypeConverter.ChangeType(values, typeof(int), true, CultureInfo.InvariantCulture, true); + result.MatchJust(out var matchedValue).Should().BeTrue("should parse successfully"); + Assert.Equal(500, matchedValue); + + } + public static IEnumerable ChangeType_scalars_source { get diff --git a/tests/CommandLine.Tests/Unit/ParserTests.cs b/tests/CommandLine.Tests/Unit/ParserTests.cs index 90147ba6..46aad457 100644 --- a/tests/CommandLine.Tests/Unit/ParserTests.cs +++ b/tests/CommandLine.Tests/Unit/ParserTests.cs @@ -847,40 +847,58 @@ public void Blank_lines_are_inserted_between_verbs() // Teardown } - [Fact] - public void Parse_default_verb_implicit() + public void Parse_repeated_options_in_verbs_scenario_with_multi_instance() { - var parser = Parser.Default; - parser.ParseArguments(new[] { "-t" }) - .WithNotParsed(errors => throw new InvalidOperationException("Must be parsed.")) - .WithParsed(args => + using (var sut = new Parser(settings => settings.AllowMultiInstance = true)) + { + var longVal1 = 100; + var longVal2 = 200; + var longVal3 = 300; + var stringVal = "shortSeq1"; + + var result = sut.ParseArguments( + new[] { "sequence", "--long-seq", $"{longVal1}", "-s", stringVal, "--long-seq", $"{longVal2};{longVal3}" }, + typeof(Add_Verb), typeof(Commit_Verb), typeof(SequenceOptions)); + + Assert.IsType>(result); + Assert.IsType(((Parsed)result).Value); + result.WithParsed(verb => { - Assert.True(args.TestValueOne); + Assert.Equal(new long[] { longVal1, longVal2, longVal3 }, verb.LongSequence); + Assert.Equal(new[] { stringVal }, verb.StringSequence); }); + } } [Fact] - public void Parse_default_verb_explicit() + public void Parse_repeated_options_in_verbs_scenario_without_multi_instance() { - var parser = Parser.Default; - parser.ParseArguments(new[] { "default1", "-t" }) - .WithNotParsed(errors => throw new InvalidOperationException("Must be parsed.")) - .WithParsed(args => - { - Assert.True(args.TestValueOne); - }); - } + using (var sut = new Parser(settings => settings.AllowMultiInstance = false)) + { + var longVal1 = 100; + var longVal2 = 200; + var longVal3 = 300; + var stringVal = "shortSeq1"; - [Fact] - public void Parse_multiple_default_verbs() - { - var parser = Parser.Default; - parser.ParseArguments(new string[] { }) - .WithNotParsed(errors => Assert.IsType(errors.First())) - .WithParsed(args => throw new InvalidOperationException("Should not be parsed.")); - } + var result = sut.ParseArguments( + new[] { "sequence", "--long-seq", $"{longVal1}", "-s", stringVal, "--long-seq", $"{longVal2};{longVal3}" }, + typeof(Add_Verb), typeof(Commit_Verb), typeof(SequenceOptions)); + Assert.IsType>(result); + result.WithNotParsed(errors => Assert.All(errors, e => + { + if (e is RepeatedOptionError) + { + // expected + } + else + { + throw new Exception($"{nameof(RepeatedOptionError)} expected"); + } + })); + } + } [Fact] public void Parse_default_verb_with_empty_name() { From 51f2fcc8f9a0aa3c93b899304dcf539675ffa445 Mon Sep 17 00:00:00 2001 From: Tim Mylemans Date: Tue, 12 May 2020 01:15:20 +0200 Subject: [PATCH 2/3] Added the option to allow multiple instances on a per option basis Renamed the global AllowMultiInstance setting to AllowMultiInstanceByDefault as this value is used to define the default action to take when the per-option AllowMultiInstance isn't specified Added extra test to test the AllowMultiInstance option on options Added extra test to make sure parsing fails on normal non(null)-AllowMultiInstance options with the global setting set to false as expected --- src/CommandLine/Core/InstanceBuilder.cs | 4 +- src/CommandLine/Core/OptionSpecification.cs | 16 ++++-- .../Core/SpecificationPropertyRules.cs | 17 +++--- src/CommandLine/OptionAttribute.cs | 52 ++++++++++++++++-- src/CommandLine/Parser.cs | 6 +-- src/CommandLine/ParserSettings.cs | 10 ++-- tests/CommandLine.Tests/Fakes/Verb_Fakes.cs | 10 ++++ tests/CommandLine.Tests/Unit/ParserTests.cs | 54 ++++++++++++++++++- 8 files changed, 137 insertions(+), 32 deletions(-) diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs index 788ce187..3810622b 100644 --- a/src/CommandLine/Core/InstanceBuilder.cs +++ b/src/CommandLine/Core/InstanceBuilder.cs @@ -46,7 +46,7 @@ public static ParserResult Build( CultureInfo parsingCulture, bool autoHelp, bool autoVersion, - bool allowMultiInstance, + bool allowMultiInstanceByDefault, IEnumerable nonFatalErrors) { var typeInfo = factory.MapValueOrDefault(f => f().GetType(), typeof(T)); @@ -120,7 +120,7 @@ public static ParserResult Build( instance = BuildImmutable(typeInfo, factory, specProps, specPropsWithValue, setPropertyErrors); } - var validationErrors = specPropsWithValue.Validate(SpecificationPropertyRules.Lookup(tokens, allowMultiInstance)); + var validationErrors = specPropsWithValue.Validate(SpecificationPropertyRules.Lookup(tokens, allowMultiInstanceByDefault)); var allErrors = tokenizerResult.SuccessMessages() diff --git a/src/CommandLine/Core/OptionSpecification.cs b/src/CommandLine/Core/OptionSpecification.cs index 77e7977f..9c420fca 100644 --- a/src/CommandLine/Core/OptionSpecification.cs +++ b/src/CommandLine/Core/OptionSpecification.cs @@ -14,10 +14,11 @@ sealed class OptionSpecification : Specification private readonly char separator; private readonly string setName; private readonly string group; + private readonly bool? allowMultiInstance; public OptionSpecification(string shortName, string longName, bool required, string setName, Maybe min, Maybe max, char separator, Maybe defaultValue, string helpText, string metaValue, IEnumerable enumValues, - Type conversionType, TargetType targetType, string group, bool hidden = false) + Type conversionType, TargetType targetType, string group, bool hidden = false, bool? allowMultiInstance = null) : base(SpecificationType.Option, required, min, max, defaultValue, helpText, metaValue, enumValues, conversionType, targetType, hidden) { @@ -26,6 +27,7 @@ public OptionSpecification(string shortName, string longName, bool required, str this.separator = separator; this.setName = setName; this.group = group; + this.allowMultiInstance = allowMultiInstance; } public static OptionSpecification FromAttribute(OptionAttribute attribute, Type conversionType, IEnumerable enumValues) @@ -45,13 +47,14 @@ public static OptionSpecification FromAttribute(OptionAttribute attribute, Type conversionType, conversionType.ToTargetType(), attribute.Group, - attribute.Hidden); + attribute.Hidden, + attribute.AllowMultiInstance); } - public static OptionSpecification NewSwitch(string shortName, string longName, bool required, string helpText, string metaValue, bool hidden = false) + public static OptionSpecification NewSwitch(string shortName, string longName, bool required, string helpText, string metaValue, bool hidden = false, bool? allowMultiInstance = null) { return new OptionSpecification(shortName, longName, required, string.Empty, Maybe.Nothing(), Maybe.Nothing(), - '\0', Maybe.Nothing(), helpText, metaValue, Enumerable.Empty(), typeof(bool), TargetType.Switch, string.Empty, hidden); + '\0', Maybe.Nothing(), helpText, metaValue, Enumerable.Empty(), typeof(bool), TargetType.Switch, string.Empty, hidden, allowMultiInstance); } public string ShortName @@ -78,5 +81,10 @@ public string Group { get { return group; } } + + public bool? AllowMultiInstance + { + get { return allowMultiInstance; } + } } } diff --git a/src/CommandLine/Core/SpecificationPropertyRules.cs b/src/CommandLine/Core/SpecificationPropertyRules.cs index 4f8b78a9..cf54c5aa 100644 --- a/src/CommandLine/Core/SpecificationPropertyRules.cs +++ b/src/CommandLine/Core/SpecificationPropertyRules.cs @@ -20,7 +20,7 @@ public static IEnumerable, IEnumerable, IEnumerable>> Lookup( IEnumerable tokens, - bool allowMultiInstance) + bool allowMultiInstanceByDefault) { return new List, IEnumerable>> { @@ -29,7 +29,7 @@ public static IEnumerable, IEnumerable, IEnumerable> EnforceSingle(IEnumerable tokens, bool allowMultiInstance) + private static Func, IEnumerable> EnforceSingle(IEnumerable tokens, bool allowMultiInstanceByDefault) { return specProps => { - if (allowMultiInstance) - { - return Enumerable.Empty(); - } - var specs = from sp in specProps where sp.Specification.IsOption() where sp.Value.IsJust() @@ -199,19 +194,19 @@ where t.IsName() join o in specs on t.Text equals o.ShortName into to from o in to.DefaultIfEmpty() where o != null - select new { o.ShortName, o.LongName }; + select new { o.ShortName, o.LongName, o.AllowMultiInstance }; var longOptions = from t in tokens where t.IsName() join o in specs on t.Text equals o.LongName into to from o in to.DefaultIfEmpty() where o != null - select new { o.ShortName, o.LongName }; + select new { o.ShortName, o.LongName, o.AllowMultiInstance }; var groups = from x in shortOptions.Concat(longOptions) group x by x into g let count = g.Count() select new { Value = g.Key, Count = count }; var errors = from y in groups - where y.Count > 1 + where y.Count > 1 && (y.Value.AllowMultiInstance == null && !allowMultiInstanceByDefault || y.Value.AllowMultiInstance == false) select new RepeatedOptionError(new NameInfo(y.Value.ShortName, y.Value.LongName)); return errors; }; diff --git a/src/CommandLine/OptionAttribute.cs b/src/CommandLine/OptionAttribute.cs index 7448b697..32e8d9eb 100644 --- a/src/CommandLine/OptionAttribute.cs +++ b/src/CommandLine/OptionAttribute.cs @@ -17,14 +17,16 @@ public sealed class OptionAttribute : BaseAttribute private string setName; private char separator; private string group=string.Empty; + private bool? allowMultiInstance; - private OptionAttribute(string shortName, string longName) : base() + private OptionAttribute(string shortName, string longName, bool? allowMultiInstance) : base() { if (shortName == null) throw new ArgumentNullException("shortName"); if (longName == null) throw new ArgumentNullException("longName"); this.shortName = shortName; this.longName = longName; + this.allowMultiInstance = allowMultiInstance; setName = string.Empty; separator = '\0'; } @@ -34,7 +36,7 @@ private OptionAttribute(string shortName, string longName) : base() /// The default long name will be inferred from target property. /// public OptionAttribute() - : this(string.Empty, string.Empty) + : this(string.Empty, string.Empty, null) { } @@ -43,7 +45,7 @@ public OptionAttribute() /// /// The long name of the option. public OptionAttribute(string longName) - : this(string.Empty, longName) + : this(string.Empty, longName, null) { } @@ -53,7 +55,7 @@ public OptionAttribute(string longName) /// The short name of the option. /// The long name of the option or null if not used. public OptionAttribute(char shortName, string longName) - : this(shortName.ToOneCharString(), longName) + : this(shortName.ToOneCharString(), longName, null) { } @@ -62,7 +64,39 @@ public OptionAttribute(char shortName, string longName) /// /// The short name of the option.. public OptionAttribute(char shortName) - : this(shortName.ToOneCharString(), string.Empty) + : this(shortName.ToOneCharString(), string.Empty, null) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// The long name of the option. + /// Should multiple instances of this option being repeated be allowed or not. + public OptionAttribute(string longName, bool allowMultiInstance) + : this(string.Empty, longName, allowMultiInstance) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// The short name of the option. + /// The long name of the option or null if not used. + /// Should multiple instances of this option being repeated be allowed or not. + public OptionAttribute(char shortName, string longName, bool allowMultiInstance) + : this(shortName.ToOneCharString(), longName, allowMultiInstance) + { + this.allowMultiInstance = allowMultiInstance; + } + + /// + /// Initializes a new instance of the class. + /// + /// The short name of the option.. + /// Should multiple instances of this option being repeated be allowed or not. + public OptionAttribute(char shortName, bool allowMultiInstance) + : this(shortName.ToOneCharString(), string.Empty, allowMultiInstance) { } @@ -82,6 +116,14 @@ public string ShortName get { return shortName; } } + /// + /// Gets if multi instancing should be allowed on this option. + /// + public bool? AllowMultiInstance + { + get { return allowMultiInstance; } + } + /// /// Gets or sets the option's mutually exclusive set name. /// diff --git a/src/CommandLine/Parser.cs b/src/CommandLine/Parser.cs index 10c9b4e1..bf8da3fe 100644 --- a/src/CommandLine/Parser.cs +++ b/src/CommandLine/Parser.cs @@ -101,7 +101,7 @@ public ParserResult ParseArguments(IEnumerable args) settings.ParsingCulture, settings.AutoHelp, settings.AutoVersion, - settings.AllowMultiInstance, + settings.AllowMultiInstanceByDefault, HandleUnknownArguments(settings.IgnoreUnknownArguments)), settings); } @@ -132,7 +132,7 @@ public ParserResult ParseArguments(Func factory, IEnumerable ar settings.ParsingCulture, settings.AutoHelp, settings.AutoVersion, - settings.AllowMultiInstance, + settings.AllowMultiInstanceByDefault, HandleUnknownArguments(settings.IgnoreUnknownArguments)), settings); } @@ -165,7 +165,7 @@ public ParserResult ParseArguments(IEnumerable args, params Type settings.ParsingCulture, settings.AutoHelp, settings.AutoVersion, - settings.AllowMultiInstance, + settings.AllowMultiInstanceByDefault, HandleUnknownArguments(settings.IgnoreUnknownArguments)), settings); } diff --git a/src/CommandLine/ParserSettings.cs b/src/CommandLine/ParserSettings.cs index 95a4cd81..ad17e268 100644 --- a/src/CommandLine/ParserSettings.cs +++ b/src/CommandLine/ParserSettings.cs @@ -25,7 +25,7 @@ public class ParserSettings : IDisposable private CultureInfo parsingCulture; private bool enableDashDash; private int maximumDisplayWidth; - private bool allowMultiInstance; + private bool allowMultiInstanceByDefaultByDefault; /// /// Initializes a new instance of the class. @@ -176,12 +176,12 @@ public int MaximumDisplayWidth } /// - /// Gets or sets a value indicating whether options are allowed to be specified multiple times. + /// Gets or sets a value indicating whether options are allowed to be specified multiple times by default. /// - public bool AllowMultiInstance + public bool AllowMultiInstanceByDefault { - get => allowMultiInstance; - set => PopsicleSetter.Set(Consumed, ref allowMultiInstance, value); + get => allowMultiInstanceByDefaultByDefault; + set => PopsicleSetter.Set(Consumed, ref allowMultiInstanceByDefaultByDefault, value); } internal StringComparer NameComparer diff --git a/tests/CommandLine.Tests/Fakes/Verb_Fakes.cs b/tests/CommandLine.Tests/Fakes/Verb_Fakes.cs index 9710d0de..9bfcca76 100644 --- a/tests/CommandLine.Tests/Fakes/Verb_Fakes.cs +++ b/tests/CommandLine.Tests/Fakes/Verb_Fakes.cs @@ -71,6 +71,16 @@ public class SequenceOptions public IEnumerable StringSequence { get; set; } } + [Verb("sequence", HelpText = "Sequence options test.")] + public class SequenceOptions_With_AllowMultiInstance + { + [Option("long-seq", true, Separator = ';')] + public IEnumerable LongSequence { get; set; } + + [Option('s', true, Min = 1, Max = 100, Separator = ',')] + public IEnumerable StringSequence { get; set; } + } + abstract class Base_Class_For_Verb { [Option('p', "patch", SetName = "mode", diff --git a/tests/CommandLine.Tests/Unit/ParserTests.cs b/tests/CommandLine.Tests/Unit/ParserTests.cs index 46aad457..000f9290 100644 --- a/tests/CommandLine.Tests/Unit/ParserTests.cs +++ b/tests/CommandLine.Tests/Unit/ParserTests.cs @@ -850,7 +850,7 @@ public void Blank_lines_are_inserted_between_verbs() [Fact] public void Parse_repeated_options_in_verbs_scenario_with_multi_instance() { - using (var sut = new Parser(settings => settings.AllowMultiInstance = true)) + using (var sut = new Parser(settings => settings.AllowMultiInstanceByDefault = true)) { var longVal1 = 100; var longVal2 = 200; @@ -871,10 +871,60 @@ public void Parse_repeated_options_in_verbs_scenario_with_multi_instance() } } + [Fact] + public void Parse_repeated_options_in_verbs_scenario_without_multi_instance_fails() + { + using (var sut = new Parser(settings => settings.AllowMultiInstanceByDefault = false)) + { + var longVal1 = 100; + var longVal2 = 200; + var longVal3 = 300; + var stringVal = "shortSeq1"; + + var result = sut.ParseArguments( + new[] { "sequence", "--long-seq", $"{longVal1}", "-s", stringVal, "--long-seq", $"{longVal2};{longVal3}" }, + typeof(Add_Verb), typeof(Commit_Verb), typeof(SequenceOptions)); + + Assert.IsType>(result); + result.WithNotParsed(errors => + { + Assert.Single(errors); + Assert.All(errors, error => + { + Assert.IsType(error); + }); + }); + } + } + + [Fact] + public void Parse_repeated_options_in_verbs_scenario_with_per_option_multi_instance() + { + using (var sut = new Parser(settings => settings.AllowMultiInstanceByDefault = false)) + { + var longVal1 = 100; + var longVal2 = 200; + var longVal3 = 300; + var stringVal = "shortSeq1"; + + var result = sut.ParseArguments( + new[] { "sequence", "--long-seq", $"{longVal1}", "-s", stringVal, "--long-seq", $"{longVal2};{longVal3}" }, + typeof(Add_Verb), typeof(Commit_Verb), typeof(SequenceOptions_With_AllowMultiInstance)); + + Assert.IsType>(result); + Assert.IsType(((Parsed)result).Value); + result.WithParsed(verb => + { + Assert.Equal(new long[] { longVal1, longVal2, longVal3 }, verb.LongSequence); + Assert.Equal(new[] { stringVal }, verb.StringSequence); + }); + } + } + [Fact] public void Parse_repeated_options_in_verbs_scenario_without_multi_instance() { - using (var sut = new Parser(settings => settings.AllowMultiInstance = false)) + using (var sut = new Parser(settings => settings.AllowMultiInstanceByDefault = false)) { var longVal1 = 100; var longVal2 = 200; From 9ea61f9aa5e40bcbc6ce1a094d26fa4d19fa2758 Mon Sep 17 00:00:00 2001 From: Tim Mylemans Date: Tue, 12 May 2020 11:30:52 +0200 Subject: [PATCH 3/3] Tiny cleanup, allowMultiInstance could be set 2 times in the constructor instead of just once --- src/CommandLine/OptionAttribute.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/CommandLine/OptionAttribute.cs b/src/CommandLine/OptionAttribute.cs index 32e8d9eb..a5c7b52a 100644 --- a/src/CommandLine/OptionAttribute.cs +++ b/src/CommandLine/OptionAttribute.cs @@ -87,7 +87,6 @@ public OptionAttribute(string longName, bool allowMultiInstance) public OptionAttribute(char shortName, string longName, bool allowMultiInstance) : this(shortName.ToOneCharString(), longName, allowMultiInstance) { - this.allowMultiInstance = allowMultiInstance; } ///