-
-
Notifications
You must be signed in to change notification settings - Fork 254
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
Add support for TypeConverter #62
Comments
Putting into the backlog for now as I don't plan to implement it myself. If someone stumbles upon this and wants to implement it, let me know. I'd accept a PR to add this. |
@natemcmaster @rubenprins I looked into this, and read #59 and #51, I'd like to confirm what to do before I start implementing it. From what I gathered from here : The main reason is to have the possibility to override how a string is parsed to the target type by putting annotations on option properties. That way we could have: [Option]
[TypeConverter(typeof(ChmodConverter))]
public int Permissions { get; set; }
[Option]
public int Count { get; set;} That would parse each property differently even though the have the same target type:
(Sorry if I misinterpreted the original example, I am not 100% sure what octal chmod is, and this was the only case where it made sense to me:) ) The problem here is parsing the same type two ways does not fit with the api of case CommandOptionType.SingleValue:
var parser = context.Application.ValueParsers.GetParser(prop.PropertyType); I assume we cannot change how the |
@nohwnd you are correct. There are two pieces of work here: allowing property-specific parser settings, and honoring TypeConverterAttribute as the way to set property-specific parsers. I think both should be possible without breaking existing API. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please comment if you believe this should remain open, otherwise it will be closed in 7 days. |
Partially addressed in #345. Need to add tests that validate this works as expected. |
Followup to #59
We should
As a bonus, we could either
The text was updated successfully, but these errors were encountered: