Skip to content
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

Make UnrecognizedArgumentHandling per command scope #371

Merged
merged 7 commits into from
Aug 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/CommandLineUtils/Attributes/CommandAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,13 @@ public string? Name
/// <summary>
/// Set the behavior for how to handle unrecognized arguments.
/// </summary>
public UnrecognizedArgumentHandling UnrecognizedArgumentHandling { get; set; } = UnrecognizedArgumentHandling.Throw;
public UnrecognizedArgumentHandling UnrecognizedArgumentHandling
{
get => _unrecognizedArgumentHandling ?? UnrecognizedArgumentHandling.Throw;
set => _unrecognizedArgumentHandling = value;
}

private UnrecognizedArgumentHandling? _unrecognizedArgumentHandling;

/// <summary>
/// Allow '--' to be used to stop parsing arguments.
Expand Down Expand Up @@ -172,7 +178,6 @@ internal void Configure(CommandLineApplication app)
app.FullName = FullName;
app.ResponseFileHandling = ResponseFileHandling;
app.ShowInHelpText = ShowInHelpText;
app.UnrecognizedArgumentHandling = UnrecognizedArgumentHandling;
app.OptionsComparison = OptionsComparison;
app.ValueParsers.ParseCulture = ParseCulture;
app.UsePagerForHelpText = UsePagerForHelpText;
Expand All @@ -181,6 +186,11 @@ internal void Configure(CommandLineApplication app)
{
app.ClusterOptions = _clusterOptions.Value;
}

if (_unrecognizedArgumentHandling.HasValue)
{
app.UnrecognizedArgumentHandling = _unrecognizedArgumentHandling.Value;
}
}
}
}
23 changes: 18 additions & 5 deletions src/CommandLineUtils/CommandLineApplication.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ internal CommandLineApplication(
SetContext(context);
_services = new Lazy<IServiceProvider>(() => new ServiceProvider(this));
ValueParsers = parent?.ValueParsers ?? new ValueParserProvider();
_parserConfig = parent?._parserConfig ?? new ParserConfig();
_parserConfig = new ParserConfig();
_clusterOptions = parent?._clusterOptions;
UsePagerForHelpText = parent?.UsePagerForHelpText ?? false;

Expand Down Expand Up @@ -255,7 +255,7 @@ public CommandOption? OptionHelp
/// </summary>
public UnrecognizedArgumentHandling UnrecognizedArgumentHandling
{
get => _parserConfig.UnrecognizedArgumentHandling;
get => _parserConfig.UnrecognizedArgumentHandling ?? Parent?.UnrecognizedArgumentHandling ?? UnrecognizedArgumentHandling.Throw;
set => _parserConfig.UnrecognizedArgumentHandling = value;
}

Expand Down Expand Up @@ -356,10 +356,23 @@ public bool ClusterOptions
/// </example>
public char[] OptionNameValueSeparators
{
get => _parserConfig.OptionNameValueSeparators;
set => _parserConfig.OptionNameValueSeparators = value;
get => _parserConfig.OptionNameValueSeparators ?? Parent?.OptionNameValueSeparators ?? new[] { ' ', ':', '=' };
set
{
if (value is null)
{
throw new ArgumentNullException(nameof(value));
}
else if (value.Length == 0)
{
throw new ArgumentException(Strings.IsEmptyArray, nameof(value));
}
_parserConfig.OptionNameValueSeparators = value;
}
}

internal bool OptionNameAndValueCanBeSpaceSeparated => Array.IndexOf(this.OptionNameValueSeparators, ' ') >= 0;

/// <summary>
/// Gets the default value parser provider.
/// <para>
Expand Down Expand Up @@ -728,7 +741,7 @@ public ParseResult Parse(params string[] args)

args ??= Util.EmptyArray<string>();

var processor = new CommandLineProcessor(this, _parserConfig, args);
var processor = new CommandLineProcessor(this, args);
var result = processor.Process();
result.SelectedCommand.HandleParseResult(result);
return result;
Expand Down
17 changes: 6 additions & 11 deletions src/CommandLineUtils/Internal/CommandLineProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ namespace McMaster.Extensions.CommandLineUtils
internal sealed class CommandLineProcessor
{
private readonly CommandLineApplication _initialCommand;
private readonly ParserConfig _config;
private readonly ArgumentEnumerator _enumerator;

private CommandLineApplication _currentCommand
Expand All @@ -31,12 +30,10 @@ private CommandLineApplication _currentCommand

public CommandLineProcessor(
CommandLineApplication command,
ParserConfig config,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, is there any point to keeping the ParserConfig class?

It seems this is sort of going the opposite direction I was hoping to go in #298. I was hoping to extract the parsing behavior of CommandLineApplication into a separate class completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to make the parser config per command, we have to associate them. If we want to inherit the config from parent command, we can’t do it inside ParserConfig as it has no info of its parent.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the parser config per command

That's probably a bigger refactor than we need right now.

With this PR as is right now, does the ParserConfig class do anything? Or can it be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ParserConfig is still needed. It is a property of CommandLineApplication. It tells the CommandLineApplication that each config is set or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can move the two properties of ParserConfig to CommandLineApplication as private fields and then delete ParserConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you want to avoid "big class". However, the UnrecognizedArgumentHandling and OptionNameValueSeparators are already published API about CommandLineApplication. It will be a breaking change if we remove it from CommandLineApplication.
As I said in #370 (comment), this PR has no open API change.

IReadOnlyList<string> arguments)
{
_initialCommand = command;
_config = config ?? throw new ArgumentNullException(nameof(config));
_enumerator = new ArgumentEnumerator(command, _config, arguments ?? new string[0]);
_enumerator = new ArgumentEnumerator(command, arguments ?? new string[0]);
CheckForShortOptionClustering(command);
}

Expand Down Expand Up @@ -260,7 +257,7 @@ private bool ProcessOption(OptionArgument arg)
}
else
{
if (_config.OptionNameAndValueCanBeSpaceSeparated)
if (_currentCommand.OptionNameAndValueCanBeSpaceSeparated)
{
if (_enumerator.MoveNext())
{
Expand Down Expand Up @@ -334,7 +331,7 @@ private bool ProcessArgumentSeparator()

private bool ProcessUnexpectedArg(string argTypeName, string? argValue = null)
{
switch (_config.UnrecognizedArgumentHandling)
switch (_currentCommand.UnrecognizedArgumentHandling)
{
case UnrecognizedArgumentHandling.Throw:
_currentCommand.ShowHint();
Expand Down Expand Up @@ -429,11 +426,9 @@ private sealed class ArgumentEnumerator : IEnumerator<Argument?>
{
private readonly IEnumerator<string> _rawArgEnumerator;
private IEnumerator<string>? _rspEnumerator;
private readonly ParserConfig _config;

public ArgumentEnumerator(CommandLineApplication command, ParserConfig config, IReadOnlyList<string> rawArguments)
public ArgumentEnumerator(CommandLineApplication command, IReadOnlyList<string> rawArguments)
{
_config = config;
CurrentCommand = command;
_rawArgEnumerator = rawArguments.GetEnumerator();
}
Expand Down Expand Up @@ -489,15 +484,15 @@ private Argument CreateArgument(string raw)

if (raw[1] != '-')
{
return new OptionArgument(raw, _config.OptionNameValueSeparators, isShortOption: true);
return new OptionArgument(raw, CurrentCommand.OptionNameValueSeparators, isShortOption: true);
}

if (raw.Length == 2)
{
return ArgumentSeparatorArgument.Instance;
}

return new OptionArgument(raw, _config.OptionNameValueSeparators, isShortOption: false);
return new OptionArgument(raw, CurrentCommand.OptionNameValueSeparators, isShortOption: false);
}

private IEnumerator<string> CreateRspParser(string path)
Expand Down
23 changes: 2 additions & 21 deletions src/CommandLineUtils/Internal/ParserConfig.cs
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
// Copyright (c) Nate McMaster.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;

namespace McMaster.Extensions.CommandLineUtils
{
/// <summary>
/// Configures the argument parser.
/// </summary>
internal class ParserConfig
{
private char[] _optionNameValueSeparators = { ' ', ':', '=' };

/// <summary>
/// Characters used to separate the option name from the value.
/// <para>
Expand All @@ -25,26 +21,11 @@ internal class ParserConfig
/// <example>
/// Given --name=value, = is the separator.
/// </example>
public char[] OptionNameValueSeparators
{
get => _optionNameValueSeparators;
set
{
_optionNameValueSeparators = value ?? throw new ArgumentNullException(nameof(value));
if (value.Length == 0)
{
throw new ArgumentException(Strings.IsNullOrEmpty, nameof(value));
}
OptionNameAndValueCanBeSpaceSeparated = Array.IndexOf(OptionNameValueSeparators, ' ') >= 0;
}
}

internal bool OptionNameAndValueCanBeSpaceSeparated { get; private set; } = true;
public char[]? OptionNameValueSeparators { get; set; }

/// <summary>
/// Set the behavior for how to handle unrecognized arguments.
/// </summary>
public UnrecognizedArgumentHandling UnrecognizedArgumentHandling { get; set; } =
UnrecognizedArgumentHandling.Throw;
public UnrecognizedArgumentHandling? UnrecognizedArgumentHandling { get; set; }
}
}
2 changes: 1 addition & 1 deletion src/CommandLineUtils/Properties/Strings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal static class Strings
public const string DefaultVersionTemplate = "--version";
public const string DefaultVersionOptionDescription = "Show version information.";

public const string IsNullOrEmpty = "Value is null or empty.";
public const string IsEmptyArray = "value is an empty array.";

public const string PathMustNotBeRelative = "File path must not be relative.";

Expand Down
22 changes: 22 additions & 0 deletions test/CommandLineUtils.Tests/CommandLineApplicationOfTTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,28 @@ public ThrowsInCtorClass()
public void OnExecute() { }
}

[Command(UnrecognizedArgumentHandling = UnrecognizedArgumentHandling.StopParsingAndCollect)]
[Subcommand(typeof(FooCommand))]
class NotThrowOnUnrecognizedArgumentClass
{
public void OnExecute() { }
}

[Command]
class FooCommand
{
}

[Fact]
public void AllowNoThrowBehaviorOnUnexpectedOptionWhenHasSubcommand()
{
using var app = new CommandLineApplication<NotThrowOnUnrecognizedArgumentClass>();
app.Conventions.UseDefaultConventions();

// should not throw
app.Execute("--unexpected");
}

[Fact]
public void ItDoesNotInitalizeClassUnlessNecessary()
{
Expand Down
30 changes: 30 additions & 0 deletions test/CommandLineUtils.Tests/CommandLineApplicationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,22 @@ public void AllowNoThrowBehaviorOnUnexpectedOptionAfterSubcommand()
Assert.Equal(unexpectedOption, arg);
}

[Fact]
public void AllowNoThrowBehaviorOnUnexpectedOptionWhenHasSubcommand()
{
var app = new CommandLineApplication
{
UnrecognizedArgumentHandling = UnrecognizedArgumentHandling.StopParsingAndCollect
};
app.Command("k", c =>
{
c.UnrecognizedArgumentHandling = UnrecognizedArgumentHandling.Throw;
});

// should not throw
app.Execute("--unexpected");
}

[Fact]
public void CollectUnrecognizedArguments()
{
Expand Down Expand Up @@ -754,6 +770,20 @@ public void OptionSeparatorMustNotUseSpace()
Assert.Equal("abc", option.Value());
}

[Fact]
public void AllowSettingOptionNameValueSeparatorsPerCommand()
{
var app = new CommandLineApplication();

app.Command("k", c =>
{
c.Option("-o", "option desc.", CommandOptionType.SingleValue);
c.OptionNameValueSeparators = new[] { '=' };
});

Assert.ThrowsAny<CommandParsingException>(() => app.Parse("k", "-o", "foo"));
}

[Fact]
public void HelpTextIgnoresHiddenItems()
{
Expand Down