-
-
Notifications
You must be signed in to change notification settings - Fork 253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds support for user defined value parsers #59
Adds support for user defined value parsers #59
Conversation
Closes natemcmaster#51. This commit allows users to supply their own value parsers via the `ValueParsers` property on `CommandLineApplication`. The bulk of the changed files are small changes like converting all the current value parser implementations to the updated interface. One side-effect of this change is that `ValueParserProvider` is no longer a singleton, but instead has state for the duration of the `CommandLineApplication`.
Any reason TypeConverters aren't used? If they would be supported, passing/conversion code could be shared more easily across projects (e.g. with MVC model binding). |
Hi @rubenprins. While I'm aware of Additionally, the interface is pretty heavy. We don't need the two-way conversion or a bunch of other stuff. Because we went with an interface implementation you could adapt your existing MVC model binders by inheriting from that same interface. Though there is something to be said for not re-inventing the wheel. I personally do not have a preference. |
Just wanted to let you know I saw this. I'll take a closer look next week. This week is pretty busy for me. @rubenprins using |
Also @atruskie - I didn't look closely, but it looks like some unit tests are failing |
@natemcmaster one advantage I see when using the TypeConverter infrastructure, is that you can annotate either the types of the properties or the properties themselves. With a type based ValueParserProvider, you lose that annotation ability (the ValueParserProvider does not receive the original PropertyInfo). For example, it's not possible to support a chmod like option (octal based) with regular integers for the other properties. With something like Note that if the ValueParserProvider were replaceable and to expose a virtual GetParser method to receive the command option's PropertyInfo, I could implement this myself. At the moment, it's not really possible to set up attribute based parsers/converters. |
@rubenprins let's use #62 to continue this. I think there are some things we can do to support TypeConverters. |
Work for natemcmaster#59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great I just had one question.
To answer yours...
This change I feel ties nicely in with #37, should we expand scope?
Let's keep this PR scoped as is. Still not sure I want to do #37 yet.
should I move the files out of the internal folder?
Yes please :)
Should we simplify the ValueTupleParserProvider code?
Can stay as is for now, but feel free to send a follow-up PR to cleanup if you want.
was forced to change the ValueParseProvider to a non-singleton
Okay with me.
added an additional public API AddRange to ValueParserProvider
Thanks. I think it's valuable enough to keep.
/// <summary> | ||
/// Gets the default value parser provider. | ||
/// <para> | ||
/// The value parsers control how arugment values are converted from strings to other types. Additional value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: argument
{ | ||
get | ||
{ | ||
throw new InvalidOperationException($"{nameof(NullableValueParser)} does not have a target type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this just return _enumType? Also, why does it say NullableValueParser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made it return _enumType
.
I chose an error because Enum
s are special cases and the EnumParser
never uses TargetType
.
Fixed spelling, removed usage of `this` (inconsistent with project style), changed EnumParser's target type a valid value - even though it is not used. Related to natemcmaster#59
See code review from natemcmaster#59
I moved the files but I was hoping for a little bit of guidance on placement or naming (sorry for being unclear). I kept the move changes separate so they're easy to track. Let me know if you want me to do more or to squash first three commits etc.. |
I wasn't too worried about namespaces and organization when this code was all internal, but now that I think about it, |
See code review from natemcmaster#59
@natemcmaster done. Should I squash? |
No need. I can squash via GitHub for you. Thanks for your effort on this. I sincerely appreciate your help. I'll be preparing the 2.2.0-rc release soon. I'll make sure to add your effort to the release notes. |
My pleasure @natemcmaster. I appreciate that you've adopted and evolved this library! It has certainly made my life easier. |
Closes #51.
This commit allows users to supply their own value parsers via the
ValueParsers
property onCommandLineApplication
. The bulk of the changed files are small changes like converting all the current value parser implementations to the updated interface.One side-effect of this change is that
ValueParserProvider
is no longer a singleton, but instead has state for the duration of theCommandLineApplication
.Questions:
ValueParsers
property onCommandLineApplication
and notCommandLineApplication<T>
ValueParserProvider
andIValueParser
are no longer internal utilities - should I move the files out of the internal folder?ValueTupleParserProvider
code? It is just a (non-)special case of an ordinary value parserValueParseProvider
to a non-singleton (parallelism in tests affected singleton state). I don't like passing the ValueParserProvider around as arguments everywhere but I thought it better than managing god states.Assumptions:
Other notes:
Parse
method allows users to customize the error string, useful for example to allow internationalization of error messagesAddRange
toValueParserProvider
. I know this wasn't agreed on so I don't mind removing it.I changed the string value for an exception message and forgot to save the file. This is why the tests fail. They all pass on my machine. Will fix test when addressing feedback.Fixed in 3a57425