dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.37k stars 378 forks source link

Cannot implement Option because the only constructors are internal #1889

Open tom-mckinney opened 1 year ago

tom-mckinney commented 1 year ago

A recent change in 2.0-beta4 removed the public constructors for Option. Because the constructors are declared with internal accessibility, 3rd party libraries and applications can no longer extend the abstract Option class.

The following constructors should be changed to protected accessibility to allow custom extensions of Option:

internal Option(
    string name,
    string? description,
    Argument argument)
    : base(description)
{
    ...
}

internal Option(
    string[] aliases,
    string? description,
    Argument argument)
    : base(description)
{
    ...
}
jonsequitur commented 1 year ago

We're trying to reduce the surface area of the API so I would want to consider a change like this carefully.

I'm curious about the use case. Is there a reason that inheriting Option<T> doesn't work for you?

tom-mckinney commented 1 year ago

@jonsequitur I appreciate the response! I've written a library that provides an attribute-based framework for declaring and configuring a System.CommandLine application. Since it's using reflection to dynamically build the arguments and options at runtime, we can't use the generic Option<T> class.

I agree with the original change to remove the public constructors (in an effort to reduce the surface area of the API), but the internal accessibility of the Option constructors seems like an oversight since Argument already has constructors with protected accessibility.

Please let me know if you have any questions or need more info.

KalleOlaviNiemitalo commented 1 year ago

Since it's using reflection to dynamically build the arguments and options at runtime, we can't use the generic Option\<T> class.

Can't you use typeof(Option\<>).MakeGenericType(type) to find the Option\ type, and then Activator.CreateInstance or some other reflection to invoke the constructor?

tom-mckinney commented 1 year ago

Can't you use typeof(Option<>).[MakeGenericType](https://learn.microsoft.com/dotnet/api/system.type.makegenerictype?view=netstandard-2.0)(_type_) to find the Option type, and then Activator.CreateInstance or some other reflection to invoke the constructor?

@KalleOlaviNiemitalo I was able to use Activator.CreateInstance like you mentioned above as a workaround. However, I still disagree with the API design of having a public abstract class that cannot be implemented by third parties. If the goal is to limit the surface area of this API exclusively to Option{T} and Argument{T} for performance reasons, I'd suggest that Option and Argument be made fully internal and to add new IOption and IArgument interfaces for composition.

On a separate note, I do agree with the spirit of #1638. I will look into converting my project to use source generators rather than runtime reflection for the parser configuration.

KalleOlaviNiemitalo commented 1 year ago

public abstract class that cannot be implemented by third parties

CompletionContext has the same restriction. I think that's even worse because its derived classes are internal so users cannot implement custom [suggest]-like directives.

pgolinski commented 1 year ago

I have exactly same problem as Tom. I have attributes which are then translated to Options and Arguments:

    [Command("build", Description = "Runs build process")]
    public class BuildCommand
    {
        public int Execute(
            [Argument("Project root directory")] string rootDir = ".",
            [Option("Output directory", "-o")] string outDir = "Build",
            [Option("Build configuration name", "-c")] string configuration = "Release",
            [Option("Target operating system [default: current OS]", "--os")] string operatingSystem = null,
            [Option("Solution directory")] string solutionDir = "Source",
            [Option("Release directory")] string releaseDir = "Release",
            [Option("A version applied to all projects")] string version = null,
            [Option("A version suffix applied to all projects")] string versionSuffix = null)

I just can't understand why I can't just create an Option or Argument with Type parameter. If you want it abstract - fine. Just let me create my own implementation which allow me to do this. Besides Argument have protected constructors and Option doesn't.

Can't you use typeof(Option<>).MakeGenericType(type) to find the Option type, and then Activator.CreateInstance or some other reflection to invoke the constructor?

I'm aware of that, but I think this way of creating objects should be avoided at all costs. It's enough to say, that changes in constructor of Option will generate errors on runtime instead of compilation errors.