From 00ae0f2254197746bf5efcfaf0502bc827aefa1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Per=20G=C3=A5rdebrink?= Date: Mon, 9 Mar 2020 14:15:50 +0100 Subject: [PATCH] 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 --- src/CommandLine/Core/InstanceBuilder.cs | 4 +-- src/CommandLine/Core/ReflectionExtensions.cs | 17 +++++++++-- .../Infrastructure/CSharpx/Maybe.cs | 8 +++++ src/CommandLine/Parser.cs | 1 - .../Mutable_Without_Empty_Constructor.cs | 19 ++++++++++++ .../Options_With_Only_Explicit_Interface.cs | 9 ++++++ .../Unit/Core/InstanceBuilderTests.cs | 14 +++++++++ tests/CommandLine.Tests/Unit/Issue591ests.cs | 29 +++++++++++++++++++ tests/CommandLine.Tests/Unit/Issue70Tests.cs | 29 +++++++++++++++++++ 9 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 tests/CommandLine.Tests/Fakes/Mutable_Without_Empty_Constructor.cs create mode 100644 tests/CommandLine.Tests/Fakes/Options_With_Only_Explicit_Interface.cs create mode 100644 tests/CommandLine.Tests/Unit/Issue591ests.cs create mode 100644 tests/CommandLine.Tests/Unit/Issue70Tests.cs diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs index 0ae564b5..dce377f1 100644 --- a/src/CommandLine/Core/InstanceBuilder.cs +++ b/src/CommandLine/Core/InstanceBuilder.cs @@ -39,7 +39,7 @@ public static ParserResult Build( Func makeDefault = () => typeof(T).IsMutable() - ? factory.MapValueOrDefault(f => f(), Activator.CreateInstance()) + ? factory.MapValueOrDefault(f => f(), () => Activator.CreateInstance()) : ReflectionHelper.CreateDefaultImmutableInstance( (from p in specProps select p.Specification.ConversionType).ToArray()); @@ -128,7 +128,7 @@ public static ParserResult Build( private static T BuildMutable(Maybe> factory, IEnumerable specPropsWithValue, List setPropertyErrors ) { - var mutable = factory.MapValueOrDefault(f => f(), Activator.CreateInstance()); + var mutable = factory.MapValueOrDefault(f => f(), () => Activator.CreateInstance()); setPropertyErrors.AddRange( mutable.SetProperties( diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs index 72e162b5..e8b7e766 100644 --- 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 index 1dacf4e8..044bb681 100644 --- a/src/CommandLine/Infrastructure/CSharpx/Maybe.cs +++ b/src/CommandLine/Infrastructure/CSharpx/Maybe.cs @@ -371,6 +371,14 @@ public static T2 MapValueOrDefault(this Maybe maybe, Func fu return maybe.MatchJust(out value1) ? func(value1) : noneValue; } + /// + /// If contains a values executes a mapping function over it, otherwise returns the value from . + /// + public static T2 MapValueOrDefault(this Maybe maybe, Func func, Func noneValueFactory) { + T1 value1; + return maybe.MatchJust(out value1) ? func(value1) : noneValueFactory(); + } + /// /// Returns an empty list when given or a singleton list when given a . /// diff --git a/src/CommandLine/Parser.cs b/src/CommandLine/Parser.cs index 8f4bd049..f801c0f7 100644 --- a/src/CommandLine/Parser.cs +++ b/src/CommandLine/Parser.cs @@ -116,7 +116,6 @@ public ParserResult ParseArguments(IEnumerable args) /// and a sequence of . /// Thrown if one or more arguments are null. public ParserResult ParseArguments(Func factory, IEnumerable 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 new file mode 100644 index 00000000..6f311bb1 --- /dev/null +++ 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 new file mode 100644 index 00000000..d367bfc0 --- /dev/null +++ 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 index a74a1af0..8dd7371c 100644 --- 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)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( + arguments); + + // Verify outcome + expected.Should().BeEquivalentTo(((IInterface_With_Two_Scalar_Options)((Parsed)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 new file mode 100644 index 00000000..3888a705 --- /dev/null +++ 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(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 new file mode 100644 index 00000000..0acc1116 --- /dev/null +++ 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(); + } + } +}