Remove constraint on T for ParseArguments with factory (#590)

* Remove constraint on T for ParseArguments with factory * Make it possible to use factory for classes without public empty constructor * Allow types with explicitly defined interface implementations * Add test for #70 * Make sure explicit interface implementations can be parsed * Add test to make sure parser can detect explicit interface implementations
commandlineparser · Mar 11, 2020 · 00ae0f2 · 00ae0f2
1 parent 19e2c95
commit 00ae0f2
Show file tree

Hide file tree

Showing 9 changed files with 124 additions and 6 deletions.
diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs
@@ -39,7 +39,7 @@ public static ParserResult<T> Build<T>(
 
             Func<T> makeDefault = () =>
                 typeof(T).IsMutable()
-                    ? factory.MapValueOrDefault(f => f(), Activator.CreateInstance<T>())
+                    ? factory.MapValueOrDefault(f => f(), () => Activator.CreateInstance<T>())
                     : ReflectionHelper.CreateDefaultImmutableInstance<T>(
                         (from p in specProps select p.Specification.ConversionType).ToArray());
 
@@ -128,7 +128,7 @@ public static ParserResult<T> Build<T>(
 
         private static T BuildMutable<T>(Maybe<Func<T>> factory, IEnumerable<SpecificationProperty> specPropsWithValue, List<Error> setPropertyErrors )
         {
-            var mutable = factory.MapValueOrDefault(f => f(), Activator.CreateInstance<T>());
+            var mutable = factory.MapValueOrDefault(f => f(), () => Activator.CreateInstance<T>());
 
             setPropertyErrors.AddRange(
                 mutable.SetProperties(

diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs
@@ -133,10 +133,21 @@ public static bool IsMutable(this Type type)
             if(type == typeof(object))
                 return true;
 
-            var props = type.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).Any(p => p.CanWrite);
-            var fields = type.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any();
+            // Find all inherited defined properties and fields on the type
+            var inheritedTypes = type.GetTypeInfo().FlattenHierarchy().Select(i => i.GetTypeInfo());
 
-            return props || fields;
+            foreach (var inheritedType in inheritedTypes) 
+            {
+                if (
+                    inheritedType.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).Any(p => p.CanWrite) ||
+                    inheritedType.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any()
+                    )
+                {
+                    return true;
+                }
+            }
+
+            return false;
         }
 
         public static object CreateDefaultForImmutable(this Type type)

diff --git a/src/CommandLine/Infrastructure/CSharpx/Maybe.cs b/src/CommandLine/Infrastructure/CSharpx/Maybe.cs
@@ -371,6 +371,14 @@ public static T2 MapValueOrDefault<T1, T2>(this Maybe<T1> maybe, Func<T1, T2> fu
             return maybe.MatchJust(out value1) ? func(value1) : noneValue;
         }
 
+        /// <summary>
+        /// If contains a values executes a mapping function over it, otherwise returns the value from <paramref name="noneValueFactory"/>.
+        /// </summary>
+        public static T2 MapValueOrDefault<T1, T2>(this Maybe<T1> maybe, Func<T1, T2> func, Func<T2> noneValueFactory) {
+            T1 value1;
+            return maybe.MatchJust(out value1) ? func(value1) : noneValueFactory();
+        }
+
         /// <summary>
         /// Returns an empty list when given <see cref="CSharpx.Nothing{T}"/> or a singleton list when given a <see cref="CSharpx.Just{T}"/>.
         /// </summary>

diff --git a/src/CommandLine/Parser.cs b/src/CommandLine/Parser.cs
@@ -116,7 +116,6 @@ public ParserResult<T> ParseArguments<T>(IEnumerable<string> args)
         /// and a sequence of <see cref="CommandLine.Error"/>.</returns>
         /// <exception cref="System.ArgumentNullException">Thrown if one or more arguments are null.</exception>
         public ParserResult<T> ParseArguments<T>(Func<T> factory, IEnumerable<string> args)
-            where T : new()
         {
             if (factory == null) throw new ArgumentNullException("factory");
             if (!typeof(T).IsMutable()) throw new ArgumentException("factory");

diff --git a/tests/CommandLine.Tests/Fakes/Mutable_Without_Empty_Constructor.cs b/tests/CommandLine.Tests/Fakes/Mutable_Without_Empty_Constructor.cs
@@ -0,0 +1,19 @@
+// Copyright 2005-2015 Giacomo Stelluti Scala & Contributors. All rights reserved. See License.md in the project root for license information.
+
+namespace CommandLine.Tests.Fakes 
+{
+    class Mutable_Without_Empty_Constructor 
+    {
+        [Option("amend", HelpText = "Used to amend the tip of the current branch.")]
+        public bool Amend { get; set; }
+
+        private Mutable_Without_Empty_Constructor() 
+        {
+        }
+
+        public static Mutable_Without_Empty_Constructor Create() 
+        {
+            return new Mutable_Without_Empty_Constructor();
+        }
+    }
+}
diff --git a/tests/CommandLine.Tests/Fakes/Options_With_Only_Explicit_Interface.cs b/tests/CommandLine.Tests/Fakes/Options_With_Only_Explicit_Interface.cs
@@ -0,0 +1,9 @@
+namespace CommandLine.Tests.Fakes
+{
+    class Options_With_Only_Explicit_Interface : IInterface_With_Two_Scalar_Options
+    {
+        bool IInterface_With_Two_Scalar_Options.Verbose { get; set; }
+
+        string IInterface_With_Two_Scalar_Options.InputFile { get; set; }
+    }
+}
diff --git a/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs b/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs
@@ -787,6 +787,20 @@ public void Specifying_options_two_or_more_times_with_mixed_short_long_options_g
             ((NotParsed<Simple_Options>)result).Errors.Should().HaveCount(x => x == expected);
         }
 
+        [Theory]
+        [InlineData(new[] { "--inputfile=file1.bin" }, "file1.bin")]
+        [InlineData(new[] { "--inputfile", "file2.txt" }, "file2.txt")]
+        public void Can_define_options_on_explicit_interface_properties(string[] arguments, string expected) 
+            {
+            // Exercize system
+            var result = InvokeBuild<Options_With_Only_Explicit_Interface>(
+                arguments);
+
+            // Verify outcome
+            expected.Should().BeEquivalentTo(((IInterface_With_Two_Scalar_Options)((Parsed<Options_With_Only_Explicit_Interface>)result).Value).InputFile);
+        }
+
+
         [Theory]
         [InlineData(new[] { "--inputfile=file1.bin" }, "file1.bin")]
         [InlineData(new[] { "--inputfile", "file2.txt" }, "file2.txt")]

diff --git a/tests/CommandLine.Tests/Unit/Issue591ests.cs b/tests/CommandLine.Tests/Unit/Issue591ests.cs
@@ -0,0 +1,29 @@
+using System.Linq;
+using CommandLine.Tests.Fakes;
+using CommandLine.Text;
+using FluentAssertions;
+using Xunit;
+using Xunit.Abstractions;
+
+//Issue #591
+//When options class is only having explicit interface declarations, it should be detected as mutable.
+
+namespace CommandLine.Tests.Unit
+{
+    public class Issue591ests
+    {
+        [Fact]
+        public void Parse_option_with_only_explicit_interface_implementation()
+        {
+            string actual = string.Empty;
+
+            var arguments = new[] { "--inputfile", "file2.txt" };
+            var result = Parser.Default.ParseArguments<Options_With_Only_Explicit_Interface>(arguments);
+            result.WithParsed(options => {
+                actual = ((IInterface_With_Two_Scalar_Options)options).InputFile;
+            });
+
+            actual.Should().Be("file2.txt");
+        }
+    }
+}
diff --git a/tests/CommandLine.Tests/Unit/Issue70Tests.cs b/tests/CommandLine.Tests/Unit/Issue70Tests.cs
@@ -0,0 +1,29 @@
+using System.Linq;
+using CommandLine.Tests.Fakes;
+using CommandLine.Text;
+using FluentAssertions;
+using Xunit;
+using Xunit.Abstractions;
+
+//Issue #70
+//When the factory overload is used for ParseArguments, there should be no constraint not having an empty constructor.
+
+namespace CommandLine.Tests.Unit
+{
+    public class Issue70Tests
+    {
+        [Fact]
+        public void Create_instance_with_factory_method_should_not_fail()
+        {
+            bool actual = false;
+
+            var arguments = new[] { "--amend" };
+            var result = Parser.Default.ParseArguments(() => Mutable_Without_Empty_Constructor.Create(), arguments);
+            result.WithParsed(options => {
+                actual = options.Amend;
+            });
+
+            actual.Should().BeTrue();
+        }
+    }
+}