Does the Options attribute class really need to be sealed?

[SJFriedl]

I have been using CommandLineParser for a while now (and it's changed how I build tools -- thank you!) including in a program with ~50 Verbs (and growing), and since many of the Verbs take common options, I was hoping to extend Options to provide common handling of them.

Simple example: if 15 Verbs take a LogFile option, each of which has a long name ("log-file") and a help description, I can do this now by constant strings to keep them consistent, but if I were able to do:

[AttributeUsage(AttributeTargets.Property)]
public class LogFileOptionAttribute : OptionAttribute
{
    public LogFileOptionAttribute(bool required = false)
        : base("log-file") // I don't use short option names for anything
    {
        HelpText = "Name of the logfile to use for blah blah blah";
        Required = required;
    }
}

and then adorn the actual option with:

public class BlahOptions {
    ...
    [LogFileOption]
    public string LogFileName {get ; set; }
    ...
}

I have easily a dozen of these common options. It's not a huge deal to create constant strings:

    [Option(OPTION_LOGFILE_NAME, HelpText = OPTION_LOGFILE_HELPTEXT)]

but adding this level of abstraction would be more convenient.

If there's a good reason for this class to be sealed, then so be it, but mine doesn't feel like a ridiculous use case. I may well get to 100 verbs for this tool by the time I'm done.

[ericnewton76]

The core code of the parser is only looking for "OptionsAttribute" not "LogFileOptionAttribute" types...

Submit a pull request with some initial work to enable this feature and we would consider it.

[SJFriedl]

I think reflection looking for an attribute will/can get derived classes of that attribute as well, but this is clearly on me to demonstrate. Will dig in.

[SJFriedl]

I finally got around to digging in, and after making literally one change - remove sealed from OptionAttribute, this all works super well.

The current code does in fact look for derived types because in Core/ReflectionExtensions.cs the GetSpecifications<T> method uses GetCustomAttributes with a true parameter: this is the inherit flag. The MSFT docs say it looks for ancestors but I believe it's actually looking for descendant classes. Maybe I'm confused about the terminology.

In any case, using (say) [LogFileOption] works great defining the long name (--log-file) and the help text in a common way, and one can still adorn it with Required=t/f flag in the usual way. This doesn't appear to impact the normal use of options, they all work as expected.

This also works, surprisingly, when LogFileOptionAttribute is defined in a different assembly than the option class that refers to it - reflection sometimes has guardrails on just how far it's going to look for things. I have a common library of supporting code that feeds multiple projects, including the command-line support.

The VerbAttribute is already non-sealed - the code shows this as having been considered but commented out - and I did not change the sealed value for ValueAttribute because I didn't need to extend it, and wanted to keep the changes limited to one thing. But I think that removing sealed from ValueAttribute is probably worth doing as well.

I'm doing my dev in Visual Studio 2022, with a large suite of C# .NET 6 code, and I compiled the CommandLine stuff with netstandard2.0; I have no real experience with cross-framework development so don't know the ins and outs of mixing and matching, and of course have not considered the F# stuff at all. I'm running my code on Windows 11.

Overall, it all Just Works(tm), and I haven't found a downside or gotcha yet.

I'm going to keep using this for a little while to shake it out, then will see if I can figure out how to make a pull request to make it so.