akamsteeg / atlex.commandlinearguments Goto Github PK

Currently, some of the class names are really long. Examples:

• WindowsStyleCommandLineArgumentsConfiguration
• LinuxStyleCommandLineArgumentsParser

We don't need to include CommandLineArguments in all names, so we can shorten those to:

• WindowsStyleConfiguration
• LinuxStyleParser

Currently, we don't set explicit sizes for the List<T>s with the built-in validators and type parsers:

Suggestion: Since we add only a single validator the default initial capacity of that list of 4 is good enough. That leaves room for validators the user adds. For the type parsers, I suggest we add the nearest power of two that's equal to or greater than the number of built-in type parsers we add. Currently, that's 11 so set a capacity of 16.

The whole symbols package stuff is replaced by SourceLink, so we should add/enable that too.

In v2.0.0 we introduced the ICommandLineArgumentsParser (#8) and IHelpWriter (#11) interfaces but for a TypeParser there's no interface. For consistency, we should add a 'ITypeParser` interface too, so users don't need get confused about sometimes having to extend an abstract class and sometimes implement an interface.

Proposal:

public interface ITypeParser
{
    /// <summary>
    /// Gets the <see cref="Type"/> this <see cref="TypeParser{T}"/> handles
    /// </summary>
    Type Type { get; }

    /// <summary>
    /// Tries to parse the specified value to the specified parse result
    /// </summary>
    /// <param name="value">
    /// The value to parse
    /// </param>
    /// <param name="parseResult">
    /// The parsed value
    /// </param>
    /// <returns>
    /// True if value was converted successfully; otherwise, false
    /// </returns>
    bool TryParse(string value, out object parseResult);
}

We must also update the configuration and all usages to support the interface instead of the current abstract base class TypeParser.

The CreateBuiltInValidators() in CommandLineArgumentsConfiguration is an instance method but can be static:

Its counterpart for the type parsers, CreateBuiltInTypeParsers(), is already static.

.NET Standard 2.0 has a much richer API and in some parts, e.g. reflection, also a lot simpler. We should investigate the possibility of moving to NET Standard 2.0 (or later) for this release.

Release steps for v2.1.0:

• Make sure al milestone 2.1.0 issues (except #40) are merged and closed
• Finish #40
• Change version numbers to 2.1.0 and update release notes (commit and push)
• Release in VSTS
• Tag release and push
• Release on Github

In #25 we're going to release v2.0.0 with a lot of breaking changes. SemVer states that we should release an update of v1.*, v1.9.0, that has ObsoleteAttributes on the to-be-removed parts of the code base.

This comment in ArgumentPropertiesHelper is not valid anymore since e254161

Implementing a new HelpWriter is quite a confusing affair. You have to override a public method and a protected method that's seemingly not called from anywhere and you get some stuff for free but no idea what.

We need to clear this mess up and making implementing this class more straight-forward and less magic.

(And do keep in mind that in #11 we're going to introduce an IHelpWriter interface)

There are quite some unused usings and redundant empty lines and whitespace in the code. This is a general issue to do some housekeeping on the code to fix those.

In #6 (e254161) we introduced TypeParser & TypeParser<T> to enable pluggable parsing of arguments. E.g. for supporting custom types. However, these are not yet covered by unit tests.

A method is not eligable for inlining when there's a throw in it.

Suggestion: Investigate the pros and cons of adding a ThrowHelper class that moves throwing exceptions out of the hot path. Implement if the results are favourable.

Example

// Before:
if (argument == null)
  throw new ArgumentNullException(nameof(argument)

// After:
if (argument == null)
  ThrowHelper.ThrowArgumentNullException(nameof(argument));

We want to provide a IEEE/Posix-style (-option, a single dash followed by a name) CommandLineArgumentsParser.

Currently, anyone who wishes to add a new CommandLineArgumentsParser needs to inherit from that abstract base class. While it's a great way to quickly whip up a new parser it also locks you into the reflection-based property setting etc.

Proposal:

To allow for more flexibility we introduce a new interface ICommandLineArgumentsParser

public interface ICommandLineArgumentsParser
{
	/// <summary>
	/// Parse the specified arguments to the specified type
	/// </summary>
	/// <typeparam name="T">
	/// The <see cref="Arguments"/> to parse to
	/// </typeparam>
	/// <param name="arguments">
	/// The arguments to parse
	/// </param>
	/// <param name="validators">
	/// The <see cref="IEnumerable{T}"/> of <see cref="ArgumentValidator"/> to validate the arguments with
	/// </param>
	/// <param name="typeParsers">
	/// The <see cref="IEnumerable{T}"/> of <see cref="TypeParser"/> to parse the argument values with
	/// </param>
	/// <returns>
	/// The <see cref="ParseResult{T}"/>
	/// </returns>
	public ParseResult<T> Parse<T>(string[] arguments, IEnumerable<ArgumentValidator> validators, IEnumerable<TypeParser> typeParsers)
		where T : Arguments, new()
}

CommandLineArgumentsParser implements this interface and in the settings we expect an ICommandLineArgumentsParser instead of the current CommandLineArgumentsParser. This leaves all current parsers intact, but it gives users more freedom when writing their own parsers.

Currently we only run benchmarks on the full .NET Framework, but since we support .NET Core via .NET Standard we should run our benchmarks on that too.

Currently, the repo has a Master and we work with feature branches from Master to add and improve and fix things. When those are done they get merged back to Master. However, that leads to an unstable Master between releases, so if we ever have to do an out-of-band release we're in trouble.

Proposal: Introduce a Development branch from Master. Feature branches will be created from Development and when we're ready for release we'll merge back to Master. Releases will be created and tagged from Master. Hotfixes will be created from Master and merged with Development.

ArgumentPropertiesHelper.FillProperty<T>(T, string, string) iterates over all properties in T to find the one with the specified name, and it sets the value for that property. However, at the call site we most likely already know the property we want to set.

Suggestion: Change the signature to FillProperty<T>(T, PropertyInfo, string) and set the value for that PropertyInfo directly.

In CommandLineArguments. TryParse<T>(string[], out T) we call the overload CommandLineArguments. TryParse<T>(string[], out T, out IEnumerable<ValidationError>) with a temporary out variable for the last parameter. However, C#7 has discards for that purpose and we should use that to make it clear we're not doing anything with that result.

We have unused usings at the following built-in TypeParser implementations:

https://github.com/akamsteeg/AtleX.CommandLineArguments/blob/cbb628c9dc80d404c4935c4fb809a1ac330580ac/src/AtleX.CommandLineArguments/Parsers/TypeParsers/CharypeParser.cs#L1-L3

Implementing a new HelpWriter is quite a confusing affair. You have to override a public method and a protected method that's seemingly not called from anywhere and you get some stuff for free but no idea what.

So, in the style of improvements to CommandLineArgumentsParser in #8 I'd like to introduce an interface IHelpWriter to give users a more flexible way to really write their own HelpWriter implementations. They can implement IHelpWriter however they want without being locked to the way HelpWriter works.

Proposed interface:

public interface IHelpWriter
{
	/// <summary>
	/// Write the help for the specified <see cref="Arguments"/> object
	/// </summary>
	/// <typeparam name="T">
	/// The type of the <see cref="Arguments"/> to write the help for
	/// </typeparam>
	/// <param name="argumentsToWriteHelpFor">
	/// The <see cref="Arguments"/> object to write the help for
	/// </param>
	void Write<T>(T argumentsToWriteHelpFor)
			where T : Arguments, new()
}

HelpWriter will implement this interface. And of course in the configuration we should accept an IHelpWriter instead of a HelpWriter.

Currently, as of v1.0.0-beta2, no documentation is generated for the release build. Therefore no documentation is included in the NuGet package. We should fix that.

We want to provide a GNU-style (--option, double dash followed by a name) CommandLineArgumentsParser.

Currently, it's the task of the ICommandLineArgumentsParser implementations to validate the arguments using the supplied ArgumentValidator instances. Since this is the same for all parsers (and it's hard to get right) we should move it out to the call site.

In #9 we reworked the configuration system and now we implicitly load all provided type parsers and validators by default. So, we don't need them to be public anymore. Now they polute the public API but that doesn't serve any purpose now.

Suggestion: Mark all default type parsers and validators as internal.

Currently, PrefixedKeyConsoleHelpWriter is an abstract class:

However, I don't think that it should be. It's a complete implementation of IHelpWriter that can work on its own. Now we're forcing people to extend it, just to call the constructor of the abstract base class PrefixedKeyConsoleHelpWriter. It's clean, because it forces strong-typed help writers, but it also makes introducing a new one for a 3rd-party thing much more involved.

Suggestion: Remove the abstract modified from PrefixedKeyConsoleHelpWriter.

The IsRequired parameter to ArgumentHelpDetails(string, string, bool) is not according to spec. It should be isRequired. That also removes the confusion IntelliSense now has.

The protected override bool TryFindRawArgumentValue(string[] allCommandLineArguments, string argumentToFind, out string value) method in WindowsStyleCommandLineArgumentsParser is case-sensitive, but all parsers should be case-insensitive unless explicitly configured as case-sensitive.

The issue is in the if (string.Compare(key, argumentToFind) == 0) on line#46. We should make that comparison case-insensitive by passing an extra argument ignoreCase: true to the Compare().

In v2.0.0 we introduced the ICommandLineArgumentsParser (#8) and IHelpWriter (#11) interfaces, but there's no IArgumentValidator interface. We still force users to inherit from ArgumentValidator. For consistency, we need to add an IArgumentValidator interface too.

Proposal:

public interface IArgumentValidator
{
/// <summary>
    /// Try Validating the argument by the specified <see cref="PropertyInfo"/>
    /// </summary>
    /// <param name="argumentPropertyInfo">
    /// The <see cref="PropertyInfo"/> of the command line argument to validate
    /// </param>
    /// <param name="isSpecified">
    /// Whether the argument is specified on the command line or not
    /// </param>
    /// <param name="originalValue">
    /// The original value, as specified on the command line
    /// </param>
    /// <param name="validationError">
    /// If the validation fails, this contains the <see cref="ValidationError"/>
    /// or null otherwise
    /// </param>
    /// <returns>
    /// True when the argument is valid, false otherwise
    /// </returns>
    bool TryValidate(PropertyInfo argumentPropertyInfo, bool isSpecified, string originalValue, out ValidationError validationError);

}

Currently, ArgumentPropertiesHelper.TryFillCustomType<T>() does a lot of things:

It find the correct TypeParser, tries to parse the parameter value and sets the corresponding property in the Arguments. We need to simplify that, and I think we can achieve that by refactoring finding the correct type parser to a separate method, because that makes it much more a 'SRP' method with a lot less levels of indentation.

The ConsoleHelpWriter displays empty parenthesis when an argument is not required:

Usage:

 /BenchmarkAssembly: The filename of the assembly with the benchmarks ()

When an argument is not required, those parenthesis should not be displayed.

Currently, the parsing of argument values is limited to the following types:

• Bool
• Byte
• Char
• DateTime
• Decimal
• Double
• Float
• Int
• Long
• Short
• String
• Any enum

We want to support custom types too.

Suggestion: Add a custom TypeParser/TypeParser<T> (abstract) base class of which the implementations can parse the string value of an argument supplied on the CLI to the specified type.

We validate an argument as soon as it's parsed, but we should defer that until after all arguments are parsed. That gives validators the possibility to be conditional, e.g. "if argument A is true, argument B should be either 1 or 2 but not 0".

This means we should also give the complete Arguments object to ValidationHelper and pass it on to the ArgumentValidator too. That makes this a breaking change.

Currently, there's a TypeParser which has an abstract bool TryParse(string, out object) method. There's also a generic TypeParser<T> that has a bool TryParse(string, out T) method. The idea behind that was to avoid boxing, but since we use reflection (with PropertyInfo.SetValue(), that uses object anyway) in ArgumentPropertiesHelper to set the value of the properties of the Arguments implementation, this isn't going to work.

Investigate: Can we remove TypeParser<T> and let all built-in type parsers just inherit from TypeParser?

Things to consider: A 3rd-party ICommandLineArgumentsParser might work without reflection and can possibly use the generic TypeParser<T> methods to avoid boxing.

When using enum-type parameters, the value is always the default value and not the value passed in from the command line.

In ArgumentPropertiesHelper<T>.TryFillPrimitiveProperty(T, PropertyInfo, string) we currently use out parameters instead of the new C#7 out variables. They make the code shorter and more readable, so I think these should be used.

I think the current settings system with CommandLineArgumentsConfiguration being some sort of base class for settings and a ConfigurationBuilder to provide a fluent interface for creating configurations is confusing.

CommandLineArgumentsConfiguration was supposed to be a base class from which users could inherit to create their own configurations. We had a DefaultCommandLineArgumentsConfiguration class in the past that did just that but that was removed in 1b2137b. However, now it's just a normal class you can instantiate and fill the properties and then pass it on. So, you can inherit from it, instantiate it or use the ConfigurationBuilder. How am I going to explain that?

So, we need something different that's easier to use but I'm not sure what. As soon as I can think of something I'm going to write it down in this issue. Any suggestions are welcome.

(Blocked by #26)

FINAL TASK BEFORE RELEASE

Checklist:

1. Create a release/v1.0.0 release branch from master
2. Ensure all feature & fix branches are merged back to release/v2.0.0
3. Update version (assembly + package) to 2.0.0 in release/v2.0.0
4. Update the release notes with all finished tasks
5. Update readme.md with changed target frameworks
6. Merge release/v2.0.0 back to master
7. Create a new v2.0.0 release on Github with the release notes
8. Tag release v2.0.0 from master
9. Push release/v1.0.0 to origin
10. Push master to origin
11. Push tag v2.0.0 to origin
12. Delete branch release/v2.0.0 locally and remote
13. Release through VSTS

Currently, CommandLineArgument.Configuration.set_Configuration() throws an incorrect InvalidOperationException when the value is null, with a weird message too:

Suggestion: Change this to a ArgumentNullException and put that into the ValidateConfiguration(CommandLineArgumentsConfiguration) method

The ArgumentPropertiesHelper and ValidationHelper can easily be static, but they aren't. Making them static reduces GC load and calling static methods is cheaper, so we should make them static.

Because these classes are internal, it doesn't change the external API so no major or minor release is needed for this.

Currently, there's a protected virtual bool ContainsHelpArgument(string[] allCommandLineArguments) in CommandLineArgumentsParser that looks for an argument help (case-insensitive). And different implementations can override it. But now when a parser does not override this with its own implementation you don't have a help argument in the same style as the other arguments.

Suggested solution: Change ContainsHelpArgument(string[]) in CommandLineArgumentsParser to protected abstract so parsers must implement it.

Pros:

• Consistency

Cons:

• Breaking API change in beta
• More implementation work when writing new parsers.

Currently, the library supports three Target Framework Monikers:

1. NET45
2. NET46
3. NETSTANDARD1.5

However, this seems a bit redundant. If we support .NET 4.5, the lowest full framework TFM we can target at the moment, we also implicitly support 4.6, 4.6.1, 4.7, etc. Why do we explicitly specify .NET 4.6 then?

With .NET Standard 1.5 we can run on the full framework in any application targeting .NET 4.6.2 or higher and in .NET Core 1.0 or higher. For consumers being stuck (for the moment?) at older versions of .NET we can give them 4.5.

So, in summary: Wouldn't it be enough to just target NET45 and NETSTANDARD1.5?

1. Ensure all feature & fix branches are merged back to release/v2.0.0
2. Update assembly version to 2.0.0 in release/v2.0.0
3. Update package version to 2.0.0-beta1 in release/v2.0.0
4. Update the release notes with all finished tasks
5. Update readme.md with changed target frameworks (#10 )
6. Create a new v2.0.0-beta1 release on Github with the release notes
7. Tag release v2.0.0-beta1 from release/v2.0.0
8. Push release/v2.0.0 to origin
9. Push tag v2.0.0-beta1 to origin
10. Release through VSTS

The class ArgumentPropertiesHelper is used to fill the properties of the specified Arguments. It's internal and it is not meant to be derived from. So, we should mark it as sealed.

Currently, CommandLineArguments has a static Configuration property to set the config. However, that means that the config can never be garbage collected and it can be huge with all the validators and type parsers.

Suggestion: Add overloads to the CommandLineArguments.TryParse() methods that take the config as a parameter. Let the current TryParse() methods use the AutoDetectConfiguration (which is the default config now).

We always create a new instance of AutoDetectConfiguration in CommandLineArguments:

Later, when the user supplies their own Configuration we don't need that anymore so it's wasted cycles and memory.

Suggestion: Create a new instance of AutoDetectConfiguration in the getter of CommandLineArguments.Configuration when the backing field _configuration is null and assign it to the field before returning it.

In ValidationHelper.TryValidate<T>(PropertyInfo, bool, string, out IEnumerable<ValidationError>) we always allocate a List<ValidationError> even when we don't have validation errors for that property:

Suggestion: Only create the List<ValidationError> when we have a validation error. Otherwise, use Enumerable.Empty<ValidationError>() to return an empty IEnumerable<ValidationError>. We need to do that because users don't expect those things to be null and they call LINQ methods on them.

Currently there's a generic package reference to System.ComponentModel.Annotations but we don't need that for the NETFX build. That one is covered by the System.ComponentModel.DataAnnotations.dll assembly included in the runtime. We should remove the package dependency for NETFX and add the assembly reference instead.

Currently, we depend on version 4.3.1 of the System.ComponentModel.Annotations package in NETSTANDARD (see #24). The latest is version 4.4.1 and we should upgrade before releasing.