Changing Parse method

[abrca]

IF YOU DON'T ANSWER THIS TEMPLATE - THE BOT WILL AUTOMATICALLY CLOSE YOUR ISSUE!

Summary

More obvious behavior

API Changes

simple program, as visual example:

namespace ConsoleApp1
{
    using System;
    using Catel.IoC;
    using Orc.CommandLine;

    public class Program
    {
        public static void Main(string[] args)
        {
            var serviceLocator = ServiceLocator.Default;
            var cmdlineParser = serviceLocator.ResolveType<ICommandLineParser>();

            var context = new MyContext();

            var validation = cmdlineParser.Parse(args, context);

            Console.WriteLine($"Database: {context.DbPath}");
        }
    }

    public class MyContext : ContextBase
    {
        [Option("db", "database")]
        public string DbPath { get; set; }
    }
}

Intended Use Case

pay attention to three lines:

            var context = new MyContext();
            var validation = cmdlineParser.Parse(args, context);
            Console.WriteLine($"Database: {context.DbPath}");

here we define new context, pass it as a value(!) to the parser, and suddenly we get it changed (using SetValue in UpdateContext in Parsers\CommandLineParser.cs)

This is very unobvious behavior, as it seems to me, violating the ideology of C#

my suggestion is:
validation becomes part of ContextBase and Parse method return new context, like:

var context = cmdlineParser.Parse(args);

if (context.validation.HasErrors)
{
	// something bad happened
	return;
}

Console.WriteLine($"Database: {context.DbPath}");```

[GeertvanHorrik]

I need to refresh my memory why we decided to implement it this way. Maybe because we use a lot of different contexts, but I still don't see any reason to pass it in. Without looking at the code, does the context specify behavior on how to parser should behave?

[abrca]

Parser fill the newly created context - original command line, parameters, detect help switch

[GeertvanHorrik]

I am happy to change this, but we need to be careful since we can't immediately break it. What if we introduce a 2nd overload, and if that works fine, we can make the other one obsolete. Interested in doing a PR?

[abrca]

Yes, I'll try

[GeertvanHorrik]

We are currently investigating some things in Orc.CommandLine. Will look into this as well.

[GeertvanHorrik]

The downside is that we can no longer pass in options of a context. Maybe we could / should solve that with CommandLineParseOptions.

[GeertvanHorrik]

Pushed a full refactoring. It's not actually a hotfix, but we are still in beta for our products so happy to take this breaking change as a hotfix and be done with it.

[GeertvanHorrik]

@abrca please review the 4.0.1 hotfix branch.

[abrca]

ok
Tomorrow only, now I am fixing my system :)

[abrca]

It works fine, quite obvious behavior.

But as you start full refactoring, I have some suggestions, based on standards of unix command-line arguments processing.
Ref:
SO
GNU Coding Standards
IEEE Std 1003.1-2017

N0.
Slightly change behavior of shortName and longName in OptionAttribute.
shortName - limit it to one char, alphanumeric (ascii).
longName - require "--" to use as prefix.
So, with
[Option("d", "debug", AcceptsValue = false)]
we'll have "-d" equal to "--debug", ("-h" equal to "--help" and so on). In shortName we can use "-" or "/", but in long - only "--" because "//debug" looks strange

N1.
Related to previous, allow joining shortNames from different options into one: "-abcd" is equal to "-a -b -c -d", implying that all options except last are with OptionAttribute.AcceptsValue = false. Last option can have value, so "-abcd value" is equal to "-a -b -c -d value"

N2.
In command-line, "--" without following alphanumeric char stop parsing. Everything till the end of line is only stored in context.OriginalCommandLine

N3.
As in GNU coding standards, hardcode "--version" - may be with renaming HelpWriterService.GetAppHeader() to .GetVersion() and adding line with copyright info.

N4. Extend "-h" handling - "/?", "-?", "--help", etc.

N5.
Unexpectedly discover, that

[Option("c", "color", AcceptsValue = true, DisplayName = "User color")]
public Color SelectedColor { get; set; }

in context, where Color is

public enum Color
{
    Undefined,
    Red,
    Green,
    Blue,
    Octarine
}

correctly recognize "-c Red" as Color.Red. But is this case seems more correct not to use enum elements names in command-line, but some text representation.
May be add some specific attribute to each enum element, like

public enum Color
{
    [Display("Fuchsia")]
    Red,
    [Display(null, ForbiddenToUse = true)]
    Octarine
}

or use Catel's DisplayAttribute
Or let programmer use some intermediate classes ?

N6.
Now HelpWriterService.GetHelp() return IEnumerable< string >. But usually help content is separated in three columns, as shown in SO - shortName, longName, HelpText.
May be let GetHelp() return IEnumerable< OptionAttribute >? In this case we can easily get three-column output in console or table in GUI application, with ability to decorate HelpText with +"(mandatory)" for (IsMandatory = true) elements, for example.

N7.
Related to previous, next step may be adding method HelpWriterService.GetUsage(), which will return single string like
utility_name [-a][-b][-c option_argument][-d|-e][-f[option_argument]] <mandatory operand>
with all available options from context, with correct decoration - square brackets for optional parameters, angle brackets - for mandatory, curly brackets for enums, e.g. [-c {Fuchsia|Green|Blue}]

In this case (for console app), if we HasWarnings in ValidationContext - we can show warnings and short .GetUsage()
And if "--help" requested show full help - GetVersion() + GetUsage() + GetHelp()

N8.
I am not sure now, but I have problems trying to use $" {} " strings in DisplayName / HelpText in OptionAttribute
I wonder, if it is possible to use LanguageService for multilingual help text in this attribute?

N9.
Make friends command-line validation with FluentValidation / Orc.FluentValidation (It's just talking, I don't go deep in it.)

[GeertvanHorrik]

Great review comments. Will try to respond on them point by point:

N0: we used to have single char, but then we ran out of options fast for some of our commands. Therefore, we implemented it as a string. Obviously, you as a developer are free to only use single characters.

N1: Due to reasoning for N0, I think this can become extremely complex. One of our use cases is to have extensions allow custom command line options. So we parse command line contexts per extension and allow an extension to respond. This will become extremely complex in such a scenario

N2 / N3 / N4 / N6 / N7: Great points. Interested in doing (separate!) PRs on this?

N5: This will make it extremely complex since the enum usage needs to be aware that it's being automated for a command line, and seems to be the wrong approach. In such case, I think it's best to create an "intermediate" option and use that instead.

N8: Not sure what you mean, but that seems like string interpolation? (see https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated)

N9: This will probably not happen. We have core validation in Catel, and don't actively use FluentValidation ourselves (but moved it over for historical reasons). We don't want to take a dependency on it.

[abrca]

Thanks for comments.

N0 (shortName as char):
Well, actually, 26 lowercase + 26 uppercase letters + 10 digits - it's quite a lot.
Also, as you can see in SO example with 'ls --help' output, there are options without short form.
But, obviously, case-sensitive options are not very popular on Windows. And also legacy... :)
You are right, requested feature can be emulated manually.

N3 ("--version") and
N4 (add more Help options):
I'll try to make PRs. hotfix/4.0.1 branch?

N8
Thanks for the hint, I found the answer - Why can't I use string interpolation in an attribute?
But it'll be nice to have multilingual help text...

[GeertvanHorrik]

I'll try to make PRs. hotfix/4.0.1 branch?

Yes please, then we will deploy 4.0.1 as if it was a 4.0 release ;-)

About N8: I believe we did add support for translating enums (DisplayAttribute in Catel). Maybe we can indeed leverage the ILanguageService of Catel for this. But how do we provide backwards compatibility to ensure that you can use fixed strings as well. Need to think about this for a while, but maybe you have a good solution in mind?

[abrca]

Example:

public class MyContext : ContextBase
{
    [Option(shortName: "c", longName: "color", DisplayName = "User color", HelpText = "Foreground color")]
    public string MyColor { get; set; }

offhand, in pseudo-code:

(1)

public class OptionAttribute : Attribute
    public string DisplayName  get => _languageService.GetString(displayName)
    public string HelpText     get => _languageService.GetString(helpText)

to avoid conflicts in translation tables (if the same word but with another meaning already have translation), some hardcoded prefix can be used
but as I see, LanguageService require parameter without spaces. Or it's just coding style?

(or 2)

use property name from context ("MyColor" in this example) as Key and:

public class OptionAttribute : Attribute
    public string DisplayName  get => _languageService.GetString("_disp" + Key)
    public string HelpText  get => _languageService.GetString("_help" + Key)

no problem with unwanted spaces (if it is a problem).
but if anyone use non-Latin naming in own code...

[GeertvanHorrik]

I think 1 is actually a good method. Then people can always implement it with keys if they prefer that instead.

[abrca]

N3 (copyright info) - PR done
if ok - add Obsolete attribute with warnings to devel branch?

N4 (add more Help options) - seems not needed - "?" already supported

[GeertvanHorrik]

No need for the obsolete stuff for now.

N4: we do support ?, but not the others (which I think are a great idea)

[abrca]

N4:
now -h, /h, -help, /help, -?, /? are supported. You mean add support to --help as in GNU Coding Standards ?
It can be done simply adding
IsSwitch("-help", singleArgument, quoteSplitCharacters) || to IsHelp method. But it'll also show help on /-help parameter, which is ugly

We can return to N0 with point longName - require "--" to use as prefix.

As you extend shortName from char to string, the difference between shortName and longName becomes minimal, so we can change longName behavior. If we agree that
shortName - string, and prefix taken from AcceptedSwitchPrefixes ("-" and "/" now),
than
longName - string, and prefix can be only "--" - because "//" or "/-" or "-/" are quite unusual.

What do you think?

BTW, while reading GNU Coding Standards, an interesting idea crystallizes - as we have some pre-defined well-known licenses - GPL, Apache, BSD, MIT, CC, etc., and --version recommends to specify license, it will be nice to keep a list with this licenses, may be from https://directory.fsf.org/wiki/Category:License + add non-free and if user choose one of them, show it in --version / --help output.

Also this feature can replace/extend Orchestra.Core\Resources\ThirdPartyNotices\

In conjunction with WebView2-preview license can be shown in windows from it's home page - no need to keep all texts, only links.

It can be integrated in Orc.LicenseManager or, may be some new Orc.LicenseHelper

To tell the truth, I don’t know if anyone will need it - but the idea is interesting

[GeertvanHorrik]

N0: not sure about this just yet, for us that would be a "massive" breaking change while the rest so far isn't. Let me think about that one a bit longer. Maybe create options for it (we have the parse options now luckily :) )

For example, we can introduce ShortNamePrefix (defaults to "-") and LongNamePrefix (defaults to "-", but can be "--"). This would also solve the /- issue for us automatically.

We do need to write unit tests for this since we have regular expressions taking care of the parsing.

Also this feature can replace/extend Orchestra.Core\Resources\ThirdPartyNotices\

I think the lib functionality will explode if we move this from Orchestra to Orc.Controls. Eventually, it's up to the caller to hook this all up together (so we might need to make the Version method virtual on the IHelpWriterService).

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[GeertvanHorrik]

I think we should no longer consider this. We should consider using System.CommandLine instead of this one (maybe mark this package as obsolete).