Repository navigation
Conversation
timcassell
left a comment
timcassell
left a comment
There was a problem hiding this comment.
Reviewed by Claude (Opus 5.5), posted by @timcassell.
728fe0f to
cf92b03
Compare
02fd109 to
38a3bdb
Compare
38a3bdb to
b0abffa
Compare
timcassell
left a comment
timcassell
left a comment
There was a problem hiding this comment.
Previous findings are addressed, thanks. One new issue in the probe predicate below.
Reviewed by Claude (Opus 5.5), posted by @timcassell.
| private static bool HasMultiValueRepeatErrorsOnly(IEnumerable<Error> errors) | ||
| { | ||
| if (errors.IsEmpty()) | ||
| return false; | ||
|
|
||
| return errors.All(error => | ||
| { | ||
| if (error is not RepeatedOptionError repeated) | ||
| return false; | ||
|
|
||
| return CommandLineOptions.IsMultiValueOption(repeated.NameInfo.LongName); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Any other error alongside a valid multi-value repeat makes the probe return false, so the real parse then reports the legal repeat as an error too:
| Args | Errors |
|---|---|
--filter A --filter B --bogus |
bogus is unknown + f, filter is defined multiple times |
--filter A --filter B --iterationCount x |
bad format + f, filter is defined multiple times |
Allow multi-instance unless a scalar option is repeated; with this, both rows report only the real error (verified against this head; the other cases in the tests are unchanged). Worth a test pairing a repeated --filter with an unknown option.
| private static bool HasMultiValueRepeatErrorsOnly(IEnumerable<Error> errors) | |
| { | |
| if (errors.IsEmpty()) | |
| return false; | |
| return errors.All(error => | |
| { | |
| if (error is not RepeatedOptionError repeated) | |
| return false; | |
| return CommandLineOptions.IsMultiValueOption(repeated.NameInfo.LongName); | |
| }); | |
| } | |
| private static bool HasNoScalarRepeatErrors(IEnumerable<Error> errors) | |
| => !errors.Any(error => error is RepeatedOptionError repeated | |
| && !CommandLineOptions.IsMultiValueOption(repeated.NameInfo.LongName)); |
| ILogger logger = NullLogger.Instance; | ||
| using (var parser = CreateParser(logger)) | ||
|
|
||
| bool allowMultiInstance; |
There was a problem hiding this comment.
Same probe block as in Parse; suggest extracting a helper, e.g. bool ShouldAllowMultiInstance(string[] args). Also .WithParsed(_ => allowMultiInstance = false) is a no-op since it's already initialized to false.
| args = expandedArgs; | ||
| using (var parser = CreateParser(logger)) | ||
|
|
||
| // First path: Detect repeated options with and `allowMultiInstance: false` setting. |
There was a problem hiding this comment.
Nit: "First path"/"Second path" → "pass"; stray "and" in "with and allowMultiInstance: false".
| using CommandLine.Text; | ||
| using JetBrains.Annotations; | ||
| using Perfolizer.Mathematics.OutlierDetection; | ||
| using System.Collections.Frozen; |
|
|
||
| private static string Escape(string input) => UserInteractionHelper.EscapeCommandExample(input); | ||
|
|
||
| internal static bool IsMultiValueOption(string longName) => MultiValueOptions.Contains(longName); |
There was a problem hiding this comment.
Contains on IReadOnlyCollection<string> is LINQ's Enumerable.Contains; it only stays case-insensitive because LINQ forwards to the HashSet. Typing the field as ISet/FrozenSet makes the lookup explicit.
| private sealed record OptionProperty(PropertyInfo Property, OptionAttribute Option); | ||
|
|
||
| /// <summary> | ||
| /// Validates that the hardcoded set in CommandLineOptions (MultiValueOptionNames) |
There was a problem hiding this comment.
Nit: MultiValueOptionNames → MultiValueOptions (also line 33).
This branch has not been deployed
2 participants
This PR contains following changes.
What's changed in this PR.
1. Enable CommandLineParser's
AllowMultiInstanceoptionThis option is introduced at CommandLineParser v2.9.1.
By enable this option, it can specify same options multiple times (e.g.
--filter A --filter Binstead of--filter A B)2. Modify
CommandLineOptions.csand add additional option mappings.Add following dictionary/hashSet definitions.
To avoid using reflection.
These mappings are manually edited (And mappings consistencies are validated by unit tests)
3. Modify
ConfigParser.csand add validation logics for duplicated scalar options.By default, when scalar option are specified multiple times. It's overwritten by last parameter.
To keep existing behavior, add logics to validate these cases.
4. Add ConfigParser related tests.