Introduce mapped options as defined in #334 #451
Conversation
TheConstructor
force-pushed
the
feature/mapped-options-v3
branch
2 times, most recently
from
457524a to
912b155
Compare
|
Could you help me on the |
|
Visual Studio is the only IDE I have found that will properly apply the quickfix for the PublicAPI analyzer. VS Code doesn't work either (OmniSharp/omnisharp-roslyn#1592). If you don't have Visual Studio, the only other path here is to update the API files by hand. Look at the existing files for examples of the syntax. I usually put new API that hasn't been released into PublicAPI.Unshipped.txt. |
|
I may do so, if you think that the change is worthy. In the end it's quite some work and I don't want to do it 'for fun' ;-) |
|
@TheConstructor up to you. As you've probably guessed, I'm not spending much time myself on this project, and certainly won't ask anyone else to. All time I spend on this project is "just for fun". Side note: this PR has a lot of changes (almost 1,200 line changing). It makes reviewing a bit harder since I probably need to dedicate a time to really think about all the changes. In my day job, I usually recommend to my team that we keep changes < 200 lines per review. I know that's not always feasible when you just need a lot of changes to build something, but if you can split the change into smaller commits or even separate pull requests, it helps me logically follow how you're piecing together the feature. |
TheConstructor
force-pushed
the
feature/mapped-options-v3
branch
from
5252ea3 to
2e049a5
Compare
Codecov Report
@@ Coverage Diff @@
## main #451 +/- ##
==========================================
- Coverage 82.19% 81.24% -0.96%
==========================================
Files 106 111 +5
Lines 3286 3481 +195
==========================================
+ Hits 2701 2828 +127
- Misses 585 653 +68
Continue to review full report at Codecov.
|
… warn --diagnostics RS0016'
- IOption for any option
- IOption{T} for options where a value other than string is offered
- IParseableOption for options directly accessible on the command-line by name
TheConstructor
force-pushed
the
feature/mapped-options-v3
branch
from
2e049a5 to
586c3c1
Compare
|
@natemcmaster I split up the commit - many lines (320) are in |
There was a problem hiding this comment.
@TheConstructor thanks for taking time to make this pull request. I finally made time to sit down and thoroughly review. I've left comments below on some specific elements of the code, however, before you jump to addressing the individual comments, here's a summary of my overall thinking.
This pull request has a lot of potential, but I can't accept this pull request in its current form. I am still interested in implementing the behaviors described in #334. In this implementation, however, the scope has expanded beyond what I think is necessary to implement #334. I take particular issue with the addition of new abstractions IOption, IParseableOption, IOption<T> and changes some common APIs which today accept CommandOption. I think adding new abstractions like that could be useful for the library -- it's something I considered doing myself -- but if we do that, that's a separate feature which requires more discussion (for instance -- could we make a unified abstraction for CommandOption and CommandArgument?)
If you're willing to do more revisions to rework it, great! Let's discuss more in the #334 thread how we could implement the behaviors we're trying to achieve. I realize we didn't close on some important design considerations. For example, would --no-option actually accept values like --no-option foo, or is --no-option considered a special type of CommandOptionType.NoValue? (I had the second behavior in mind, but I didn't explicitly say so or justify this in the conversation in #334.)
If don't want to spend more time on this, let's close this one.
Either way, please let me know how you'd like to proceed.
Thanks,
Nate
| { | ||
| var constantValueOption = new ConstantValueOption<T>(value); | ||
| _mappings.Add(constantValueOption); | ||
| _commandLineApplication.AddOption(constantValueOption); |
There was a problem hiding this comment.
I think we should invert the relationship here. CommandOption/Argument are decoupled from CommandLineApplication. I'd like to see if we can do the same for MappedOption. The existing model is that option/arg objects can be constructed independently of the CommandLineApplication object, and then are added to it.
There was a problem hiding this comment.
The tricky thing for me was adding it to both places. I didn't want to change the parsing logic to take a list of strings for each argument style (short, long, symbol) - which I thought would be required to otherwise ensure that each possible flow of actions would lead to a working construction.
| /// <param name="template">parameter-template e.g. <code>-a|--apple</code></param> | ||
| /// <param name="value">value to use if the mapped parameter was specified</param> | ||
| /// <returns>the mapping</returns> | ||
| public ConstantValueOption<T> Add(string description, string template, T value) |
There was a problem hiding this comment.
Let's re-order the arguments to string template, string, description, T value for consistency with CommandLineApplication.Option(string template, string, description,... and .Argument(string template, string, description,)
| public MappedOption<T> Map(string template, T value) | ||
| { | ||
| Add(template, value); | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Add mapping | ||
| /// </summary> | ||
| /// <param name="description">description for use in the help-text</param> | ||
| /// <param name="template">parameter-template e.g. <code>-a|--apple</code></param> | ||
| /// <param name="value">value to use if the mapped parameter was specified</param> | ||
| /// <returns><code>this</code></returns> | ||
| public MappedOption<T> Map(string description, string template, T value) | ||
| { | ||
| Add(description, template, value); | ||
| return this; |
There was a problem hiding this comment.
I think users may find this method a bit confusing. When should a user call Add vs Map? They are both doing the same thing, except one can be chained. Can we remove one of these methods?
There was a problem hiding this comment.
Sure, I was undecided, so I added both ^^; Do you prefer one style?
| private IEnumerable<T> MappedValues => | ||
| _mappings.Where(m => m.HasValue()) | ||
| .Select(m => m.ParsedValue); | ||
|
|
||
| /// <inheritdoc /> | ||
| public T ParsedValue => MappedValues.FirstOrDefault(); |
There was a problem hiding this comment.
This makes me think we have the wrong abstraction for IParseableOption. What if there are two parsed values? Why are we only selecting the first?
There was a problem hiding this comment.
Again there is ParsedValues just below
| .FirstOrDefault(); | ||
| } | ||
|
|
||
| void IOption.Reset() |
There was a problem hiding this comment.
Why is this using an explicit interface implementation and not a normal method definition?
| public void AddOption(CommandOption option) | ||
| { | ||
| AddOption((IOption)option); | ||
| } | ||
|
|
||
| internal void AddOption(IOption option) | ||
| { | ||
| _options.Add(option); | ||
| } |
There was a problem hiding this comment.
| public void AddOption(CommandOption option) | |
| { | |
| AddOption((IOption)option); | |
| } | |
| internal void AddOption(IOption option) | |
| { | |
| _options.Add(option); | |
| } | |
| public void AddOption(IOption option) | |
| { | |
| _options.Add(option); | |
| } |
The cast and extra method hop appears unnecessary to me.
| /// <inheritdoc /> | ||
| public ValidationResult GetValidationResult(IOption option, ValidationContext context) | ||
| { | ||
| if (option.Values.Count > 1) |
There was a problem hiding this comment.
This validator seems to be unnecessary. As user of the API, I would expect this to be enforced already through using CommandOptionType.SingleValue.
| .And.OnlyContain(o => string.IsNullOrEmpty(o.ShortName)); | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
Can you delete commented-out code?
| { | ||
| var app = Create<ShortNameOverride>(); | ||
|
|
||
| using var assertionScope = new AssertionScope(); |
There was a problem hiding this comment.
I personally think usage of AssertionScope usually indicates a test is actually doing more than one thing. As a matter of preference, I prefer to have more and smaller test methods which test a single behavior.
| } | ||
| } | ||
|
|
||
| private class PrivateSetterProgram |
There was a problem hiding this comment.
This, and the next 130 lines of code, appear to be copy/pasted from OptionAttributeTests. Can you delete instead of duplicating this content?
|
@TheConstructor thank you for all of your effort on this feature. At this point, I am going to close this request. I have decided to put this project into maintenance mode. So, I won't be taking new features or any other major changes. I've posted my updated thoughts in #485. Thanks again for the effort! |
So this is how I propose to implement #334. I hope I will get the last details right in the next days, as at least one exception needs another text (annotation with value not castable to property type). I also may add another test or two.
@natemcmaster @JakeSays: As you participated in the discussion on #334, please have a look, if the mechanics are as expected.