Add support for guid options - #345
Conversation
natemcmaster
left a comment
natemcmaster
left a comment
There was a problem hiding this comment.
Good job! This looks good and is a job well done for a first PR ever. Thanks for the contribution!
A few things
- As this code depends on an API (TypeDescriptor) which is unavailable in .NET Standard 1.6, I'm unable to backport this to the 2.6 release that I was planning to release soon (cref #338). This will be in the 3.0 release which I hope to get done sometime in spring. After this merges, I'll post a link to the custom nuget feed where I publish nightly builds so you can try pulling this into your own project.
- Are you okay if I add your username to the release notes and CHANGELOG to thank you for your work?
- I have a few questions about behavior, as posted below.
Thanks again,
Nate
| return true; | ||
| } | ||
|
|
||
| var converter = TypeDescriptor.GetConverter(targetType); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| var converter = TypeDescriptor.GetConverter(targetType); | ||
| if (converter.CanConvertFrom(typeof(string))) | ||
| { | ||
| if (_parsersByTargetType.Count >= _maxCacheCapacity) |
There was a problem hiding this comment.
Why is there a limit to 100 type converters?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is fine by me. This component is private anyway.
| @@ -0,0 +1,123 @@ | |||
| namespace McMaster.Extensions.CommandLineUtils.Abstractions | |||
| { | |||
| using System; | |||
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Can Roslyn based extensions help with enforcing the chosen code style?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Not really. But it is something I am considering to look into, when the time comes.
I am very happy to hear that and definitely positive to be mentioned in the release notes and CHANGELOG . Thanks! |
|
Thanks for the contribution! I'll make sure to add you to the release notes 😄 |
2 participants
Fix to #333 "System.InvalidOperationException: on parsing GUID command line option"