Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions src/CommandLineUtils/Abstractions/DefaultValueParserFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
namespace McMaster.Extensions.CommandLineUtils.Abstractions
{
using System;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the formatting of your code doesn't exactly match with the rest of the project, but I don't blame you -- Visual Studio (and .NET in general) don't provide me with good ways to help new contributors follow a consistent code format. After this is merged, I'll reformat to fit with the rest of the project. That will change a few things, such as moving usings to the top of the file, adding accessibility modifiers, removing regions, and adding braces to conditional code. Of course, you're welcome to make those changes yourself and push and update to this PR, but it's not a requirement :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can Roslyn based extensions help with enforcing the chosen code style?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Know of any good ones? My criteria is that I need a tool that works on my macbook and can be easily used by all contributors without installing something heavy like ReSharper. I've toyed with integrating dotnet-format into the build.ps1 script, but it's not very good yet and always gives me false warnings on my macbook due to it's broken implementation of loading the MSBuild project system.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. But it is something I am considering to look into, when the time comes.

using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.Globalization;
using System.Text;

/// <summary>
/// A factory creating generic implementations of <see cref="IValueParser{T}"/>. The implementations are based
/// on automatically located <see cref="TypeConverter"/> classes that are suitable for parsing.
/// </summary>
sealed class DefaultValueParserFactory
{
const int DefaultMaxCacheCapacity = 100;

#region Private Fields

private readonly int _maxCacheCapacity;

[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)]
readonly Dictionary<Type, IValueParser> _parsersByTargetType = new Dictionary<Type, IValueParser>();

[DebuggerBrowsable(DebuggerBrowsableState.Never)]
readonly HashSet<Type> _notSupportedTypes = new HashSet<Type>();

#endregion

public DefaultValueParserFactory(int maxCacheCapacity = DefaultMaxCacheCapacity)
{
_maxCacheCapacity = maxCacheCapacity;
if (_maxCacheCapacity < 1) throw new ArgumentOutOfRangeException(nameof(maxCacheCapacity));
}

[DebuggerStepThrough]
public bool TryGetParser<T>(out IValueParser<T> parser)
{
if (TryGetParser<T>(out IValueParser generalizedParser))
{
parser = (IValueParser<T>)generalizedParser;
return true;
}

parser = null;
return false;
}

public bool TryGetParser<T>(out IValueParser parser)
{
var targetType = typeof(T);
if (_notSupportedTypes.Contains(targetType))
{
parser = null;
return false;
}

if (_parsersByTargetType.TryGetValue(targetType, out parser))
{
Debug.Assert(targetType == parser.TargetType);
return true;
}

var converter = TypeDescriptor.GetConverter(targetType);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used this API before. Do you know what other kinds of types this supports? Also, does this incidentally implement support for TypeConverterAttribute? If so, this is a two-birds-one-stone PR and also resolves #62.

@AlexeyEvlampiev AlexeyEvlampiev Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the change made does address #62. The requested functionality is implemented in the form of an implicit fallback, when no custom value parser is registered for an argument type. In principle the change introduced support for a wide range of CLR types (e.g. TimeSpan, Version etc.). And yes, your right. The TypeConverterAttribute functionality is enabled with this change. So custom types annotated with TypeConverterAttribute will be parsed automatically.

if (converter.CanConvertFrom(typeof(string)))
{
if (_parsersByTargetType.Count >= _maxCacheCapacity)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a limit to 100 type converters?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting the cache capacity prevents the excessive memory consumption under stress loads or invocations triggered by a malfunctioning caller. I subjectively chosen 100 as the requirement to support more than 100 different argument types per command line application is unlikely. Still, the component is capable of handling these exotic cases with some performance penalty (approximately 3 times slower).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoken like a perf-minded engineer. :) I see where you are coming from, but I think this might be premature optimization. It appears that the implementation of TypeDescriptor is already caching its results and has some memory optimizations under the hood (like using WeakReference). (see https://github.com/dotnet/runtime/blob/master/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs) I think we can simplify this code by skipping the cache and count checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fine by me. This component is private anyway.

_parsersByTargetType.Clear();
parser = new TypeConverterValueParser<T>(targetType, converter);
_parsersByTargetType[targetType] = parser;
Debug.Assert(_parsersByTargetType.Count <= _maxCacheCapacity);
return true;
}

parser = null;
if (_notSupportedTypes.Count > _maxCacheCapacity)
_notSupportedTypes.Clear();
_notSupportedTypes.Add(targetType);
Debug.Assert(_notSupportedTypes.Count <= _maxCacheCapacity);
return false;
}

public IValueParser<T> GetParser<T>()
{
return TryGetParser<T>(out IValueParser<T> converter)
? converter
: throw new NotSupportedException(
new StringBuilder($"No suitable type converter found for {typeof(T)}.")
.Append($" Make sure a type converter capable of parsing {typeof(string)} to {typeof(T)} exists and is discoverable.")
.Append($" Did you forget to annotate the target type with {typeof(TypeConverterAttribute)}?")
.ToString());
}

private sealed class TypeConverterValueParser<T> : IValueParser<T>
{
public TypeConverterValueParser(Type targetType, TypeConverter typeConverter)
{
TargetType = targetType ?? throw new ArgumentNullException(nameof(targetType));
TypeConverter = typeConverter ?? throw new ArgumentNullException(nameof(typeConverter));
}

public Type TargetType { get; }

private TypeConverter TypeConverter { get; }

public T Parse(string argName, string value, CultureInfo culture)
{
try
{
culture ??= CultureInfo.InvariantCulture;
return (T)TypeConverter.ConvertFromString(null, culture, value);
}
catch (ArgumentException e)
{
throw new FormatException(e.Message, e);
}
}

object IValueParser.Parse(string argName, string value, CultureInfo culture) => Parse(argName, value, culture);
}

}
}
4 changes: 4 additions & 0 deletions src/CommandLineUtils/Abstractions/ValueParserProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace McMaster.Extensions.CommandLineUtils.Abstractions
public class ValueParserProvider
{
private readonly Dictionary<Type, IValueParser> _parsers = new Dictionary<Type, IValueParser>(10);
private readonly DefaultValueParserFactory _defaultValueParserFactory = new DefaultValueParserFactory();

internal ValueParserProvider()
{
Expand Down Expand Up @@ -100,6 +101,9 @@ public IValueParser GetParser(Type type)
return EnumParser.Create(type);
}

if (_defaultValueParserFactory.TryGetParser<T>(out parser))
return parser;

if (ReflectionHelper.IsNullableType(type, out var wrappedType) && wrappedType != null)
{
if (wrappedType.IsEnum)
Expand Down
35 changes: 35 additions & 0 deletions test/CommandLineUtils.Tests/ValueParserProviderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ private class Program

[Option("--timespan")]
public TimeSpan TimeSpan { get; }

[Option("--guid", CommandOptionType.SingleValue)]
public Guid Guid { get; }

[Option("--guid-opt", CommandOptionType.SingleValue)]
public Guid? GuidOpt { get; }
}

private sealed class InCulture : IDisposable
Expand Down Expand Up @@ -480,6 +486,35 @@ public void ParsesBoolArray(int repeat)
Assert.All(parsed.Flags, value => Assert.True(value));
}

[Theory]
[InlineData("ff23ef12-500a-48df-9a5d-151c2adc2a0a")]
[InlineData("ff23ef12500a48df9a5d151c2adc2a0a")]
[InlineData("{ff23ef12-500a-48df-9a5d-151c2adc2a0a}")]
[InlineData("(ff23ef12-500a-48df-9a5d-151c2adc2a0a)")]
[InlineData("{0xff23ef12,0x500a,0x48df,{0x9a,0x5d,0x15,0x1c,0x2a,0xdc,0x2a,0x0a}}")]
public void ParsesGuid(string arg)
{
var expected = Guid.Parse("ff23ef12-500a-48df-9a5d-151c2adc2a0a");
var parsed = CommandLineParser.ParseArgs<Program>("--guid", arg);
Assert.Equal(expected, parsed.Guid);
}

[Theory]
[InlineData("ff23ef12-500a-48df-9a5d-151c2adc2a0a")]
[InlineData("ff23ef12500a48df9a5d151c2adc2a0a")]
[InlineData("{ff23ef12-500a-48df-9a5d-151c2adc2a0a}")]
[InlineData("(ff23ef12-500a-48df-9a5d-151c2adc2a0a)")]
[InlineData("{0xff23ef12,0x500a,0x48df,{0x9a,0x5d,0x15,0x1c,0x2a,0xdc,0x2a,0x0a}}")]
[InlineData("")]
public void ParsesGuidNullable(string arg)
{
var expected = String.IsNullOrWhiteSpace(arg)
? (Guid?)null
: Guid.Parse("ff23ef12-500a-48df-9a5d-151c2adc2a0a");
var parsed = CommandLineParser.ParseArgs<Program>("--guid-opt", arg);
Assert.Equal(expected, parsed.GuidOpt);
}

[Theory]
[InlineData(nameof(Program.Float), "--float", "123.456,7", "de-DE", 123456.7f)]
[InlineData(nameof(Program.Double), "--double", "123.456,789", "de-DE", 123456.789)]
Expand Down