Propagate parser-settings to HelpText (fix #414 and #455)

[x789]

This fixes issues #414 and #455 (value of AutoHelp and AutoVersion are ignored by HelpText)
This fix does not change the public interface. It is achieved by overloading HelpText.AutoBuild. If a ParameterSettings object is passed, the values it contains are used. If no ParameterSettings are passed to AutoBuild, help and version are always displayed.

[NeilMacMullen]

Looks like we were heading in similar directions. Still waiting to hear back on my PR #458 but would be sensible to merge these at some point.

[moh-hassan]

Thanks @x789 for your effort on this PR.
This help in fixing the issues: #414 and #455 without Breaking Change.

[NeilMacMullen]

@moh-hassan

******* PLEASE DO NOT MERGE ******

This change has been merged into #458

[x789]

@moh-hassan,

setting AutoVersion and AutoHelp directly on HelpText works fine.

#414 and #455 describe a different issue: The settings of a Parser are not regarded when the help-text is automatically generated.
Thus, if you create a Parser and set its AutoHelp to false, it will be displayed on the auto-generated help-text. Please check out this code for a demonstration of these issues. You will see that the ‘help’ option is still displayed if the help text is generated automatically (when using v2.5.0).

[moh-hassan]

@x789, That is true as you said.

Based on your example, It seems that in v2.4.3 and above, configuring parser with AutoHelp = false, cause other error and the help screen show twice ERROR(S) and it's supposed to show help without errors.

ERROR(S):
Required option 'i, input-file' is missing.

I forked your test case to show 4 different behavior

The valid behaviour is by avoiding configuring parser with AutoHelp

I think it's better to fix this bug and unify the generation of help either from parser setting or help setting.
I found that you fix the bug by using an overloading of AutoBuild

        var settings = new ParserSettings() { AutoHelp = false, AutoVersion = false };
        var helpText = HelpText.AutoBuild(parserResult, settings);

Can you modify your PR to fix the bug described above and unify the generation of help either from parser setting or help setting for all AutoBuild overloads.

Note: , There are 18 test cases for HelpText have been added.

[NeilMacMullen]

@moh-hassan @x789 This PR has already been merged into my PR #458 (as you requested). Making further modifications on this branch seems a bit pointless assuming you are willing to accept my PR ! ;-)

[moh-hassan]

@NeilMacMullen
As your PR #458 is based on this PR, there is still a bug as described above.
We can avoid the modification of PR #461 and concentrate for the fix in your PR.
Can you migrate that example that configure both parser and HelpText by using your PR.
This can help developers to have a clean migration path from current help system to yours, specially most open issues are related to Help System.
As you know there was not a clean migration path from help system in v1.9.x to v2.x and we are in need to avoid this in the next V3.

[NeilMacMullen]

@moh-hassan that bug is not present on my branch. I have added a test to verify that.

[moh-hassan]

@NeilMacMullen , Good that the bug is not present in your PR.
Just, i want to try the migration to your PR, can you migrate that example and post here as an attach file.
I use your new package in CI server to run the migrated code.

[NeilMacMullen]

@moh-hassan
Ok I think the 'new' version would look like this. There's no need to call SetTextWriter if you're happy for HelpText to go to the console

                new Parser(c =>
                {
                    c.EnableDashDash = true;
                    c.CaseInsensitiveEnumValues = false;
                    c.IgnoreUnknownArguments = true;
                })
                .SetAutoHelp(false)
                .SetAutoVersion(false)
                .SetHelpTextConfiguration(c => { c.AdditionalNewLineAfterOption = false; })
                .ParseArguments<Options>(args)
                .WithParsed<Options>(options => Run(options));

[moh-hassan]

Sweet Fluent interface Code :)
It's better for redirecting errors to Console.Error and others to Console.Out with user configuration. That is one of the needs for most developers.

[NeilMacMullen]

:-) Yes - you can just add a .SetTextWriter(Console.Error) to the chain if you want to do that

[x789]

                new Parser(c =>
                {
                    c.EnableDashDash = true;
                    c.CaseInsensitiveEnumValues = false;
                    c.IgnoreUnknownArguments = true;
                })
                .SetAutoHelp(false)
                .SetAutoVersion(false)
                .SetHelpTextConfiguration(c => { c.AdditionalNewLineAfterOption = false; })
                .ParseArguments<Options>(args)
                .WithParsed<Options>(options => Run(options));

@NeilMacMullen, please don't get me wrong. I admire your commitment to this project and your approach.

I personally like 'fluent' interfaces. But for me, the configuration quoted above is not symmetric. I'm afraid that users can get confused because they have to use two different types of configuration, an Action<ParserSettings> and methods like SetAutoHelp. What happens if I configure Parser to disable auto-help and configure HelpText to display auto-help?

  new Parser(c => { c.AutoHelp = false })
    .SetAutoHelp(true)
    [...]

But maybe I have wrong expectations for the class Parser. When I use it, I don't want to have to deal with help text options. I expect an automatically generated help to take the parser settings into account without having to worry about it.

[NeilMacMullen]

@789 Actually I agree with youand appreciate the comments :-) I had already decided to provide fluent modifiers for the parser settings. , not appreciating how they used up to now. (One of the key changes in my PR is to differentiate between the settings used when parsing and the setting used when generating the error text. FWIW, I can't imagine myself every changing AutoHelp/Version but I did want the error text to include the names of acceptable enum values which is currently quite clumsy using the old 'AutoBuild' version.

I'm not particularly happy that SetHelpTextConfiguration is required but that's an unfortunate consequence of "burying" a bunch of useful formatting flags in the HelpText class. In the ideal world, these would be hoisted up and the Parser class would be the only thing that users would have to know about. I'm open to ideas on better ways to expose these.

Your point about contradictory configurations is good but 1) it's also a problem (actually worse) in the original design and 2) only exists in the new design because of the desire to allow backwards compatibility. I've actually marked all the old methods as Obsolete in my PR but would be quite happy to remove them and call this a breaking change.

[moh-hassan]

@x789 ,
@NeilMacMullen
I did more investigation and found that the objective of AutoHelp and AutoVersion is different than show/hide objective.

Added AutoHelp and AutoVersion properties to control adding of implicit 'help' and 'version' options/verbs

That is the original PR #256 with commit 5dad6e and merged in v2.3.0 with commit 0db898d

It covers the issues #87 and #200 which are not related to issues #414 and #455 and the test cases used have different Assertion objective.

What are your suggestions ?

[NeilMacMullen]

@moh-hassan Maybe I'm missing something but I don't think the behaviour obtained with the latest changes differs from that desired in #256? In the latest changes, if you specify AutoHelp/AutoVersion=false then:

• the help/version verbs are not added to the parseable list and are treated as unrecognised items if the user tries to supply them
• help/version is not listed in the helptext.

Are you suggesting that something else should happen?

[moh-hassan]

#256 (comment)

[moh-hassan]

@NeilMacMullen

• I want to confirm that we have a common understanding for the AutoHelp/AutoVersion feature and it should be used as given in PR Added AutoHelp and AutoVersion properties to control adding of implicit 'help' and 'version' options/verbs #256.
• It's nice to get the consideration mentioned @x789 into account.
• The Fluent Interface API (FI) is a good enhancement and can be other API of using the library and it's best to be built in the same common public API in HelpText without the need to be obsolete. Any bugs in HelpText can be fixed in its place without the effect of FI.
• I try to understand the intent of the change in the issues Incorrect help text if AutoVersion = false #414 and Help Text Displays 'help' Option/Verb if AutoHelp is 'False' #455 and I'll feedback.

[moh-hassan]

There is a need for displaying help text with fluent syntax? #39

[moh-hassan]

@x789
@NeilMacMullen

I have added a wiki documentation page in my fork to explain the usage of AutoHelp / AutoVersion properties of the parser with code example and four fiddle demos.
I hope It can help in understanding the PR #256

[moh-hassan]

I think that #414 and #455 are now a new feature of hiding Version/help option and are not considered as a bug.
Is't right?

[x789]

#256 describes the possibility to automatically add a 'help' and 'version' verb/option to the parser. This works as far as only the parser is considered. I think when #256 was integrated, it was not considered that there will also be applications without 'help' and 'version' (see #294, #414 and #455).

@NeilMacMullen, @moh-hassan I do not want to stop the discussion about the fluent interface. But we should discuss it as part of #458. I am also confident that we will find a way to introduce a new API without breaking the old one.

@moh-hassan The wiki page looks okay to me. It describes the status quo. But it does not cover applications without 'help' and 'version' verbs/options.

I consider #414 and #455 to be bugs since the auto-generated help displays verbs/options that are not available. This is confusing and avoids that we can use the auto-help text in applications that do not have 'help' or 'version' verbs/options. If I interpret #294 correctly, others have come across this aspect as well.
v2.5.0 behaves as described in section 'Reproduction Steps' of #455:

2. Execute your program with `--help`
   > ERROR(S):
   > Verb '--help' is not recognized.
   > --help       Display this help screen.
   > --version    Display version information.

[NeilMacMullen]

@x789 - yes I agree that most of this discussion should move to #458 - the only relevance here is that #458 aims to remove the requirement for the user ever to use or know about AutoBuild and therefore uses your changes in a slightly modified version. (Hence I'm keen to know which direction we are going with this - if this PR is merged then I'll just end up having to redo changes.)

@moh-hassan
I'm pretty sure that both this PR and #458 are correctly interpreting the original intention behind introducing the AutoHelp and AutoVersion flags which was to treat these as flags which suppress the automatic generation of the associated commands and options. The hypothetical user of AutoHelp=false would be expected to

1. Supply their own "help" verb (assuming the wanted one)
2. Suppress the standard AutoBuild "verb not recognised" command by setting TextWriter to null and/or using WithNotParsed to output their own first level help screen.

On you wiki you seem to be implying that the suppression of AutoBuild output might happen automatically if AutoHelp was set to false. Personally I think that is a step too far and undesirable for users who want the automatically generated basic usage but who want to supply their own "help" behaviour.

I tried your Fiddle and couldn't get to output anything sensible. For example, I commented out the "with.AutoVersion=false;" line and the WithNotParsed line and got absolutely no output. Is that based on the current state of the develop branch?

[NeilMacMullen]

@moh-hassan Played some more with your Fiddle and I think 2.5 is broken in some ways that have nothing to do with AutoHelp…

1. https://dotnetfiddle.net/uTUhKc - dual error line (nothing to do with AutoHelp)
2. https://dotnetfiddle.net/BZgB4M - missing autobuild output when not setting config flags
3. https://dotnetfiddle.net/gz1Rej - missing autobuild output even when AutoHelp set to true explicitly

It looks more like the explicit Parser configuration mechanism is broken in this release so I'm not sure the current behaviour is really a good guide for how it should work. If I had to guess, I would assume that when calling the constructor with a no-op function, the ParserSettings object is a new one with a null TextWriter rather than the settings used by Parser.Default. This seems like a very bad disconnect - Parser.Default should give equivalent behavior to new Parser(_=>{}).

[moh-hassan]

@NeilMacMullen

I think 2.5 is broken in some ways that have nothing to do with AutoHelp…

The AutoHelp/AutoVersion feature is added in v2.4.3 as described here and no break in v2.5.
You can try v2.4.3 and you get the same result.
Can you point me to a commit that may cause a break of that feature.

The feature is working as intended and tested based on the requirement of @nemec (one of the Authors of the library)

@nemec commented on Mar 26, 2018

I think this solves the issues nicely. Can you add a few tests to help us prevent regressions? I think the >following should cover things, but if you think of any other scenarios feel free to include them:

AutoHelp = false and invoking --help returns an error.
AutoVersion = false and invoking --version returns an error.
AutoHelp = false, a flag for --help added manually by the user to the Options class, and it can be invoked successfully.
AutoVersion = false, a flag for --version added manually by the user to the Options class, and it can be invoked successfully.

The tests in the original commit here and in v2.5.0 here

For fiddle, there is a problem in redirection of console.Error and duplicate the errors
I run your fiddle cases in Linqpad and there is no duplication of errors.
For custom Parser, HelpWriter = null by default so you get no output, this is working

            var parser = new CommandLine.Parser(c=>{c.HelpWriter=Console.Out;});

So AutoHelp/AutoVersion is working as intended and approved by the @nemec.
If something wrong in this feature we can discuss it and provide a solution.

[NeilMacMullen]

@moh-hassan I must admit I am very confused ;-) You were the one who suggested using Fiddle to try and explore the behaviour, with the implication that this PR (and therefore mine) were somehow getting it wrong. Since regression tests are passing on both PRs I'm not really sure what you think should be changed! Perhaps if you could point to a failing test case it would be clearer? (Maybe there is one in an unmerged PR?) Otherwise we just seem to be going round in circles....

The fact that the "default" new Parser constructor behaviour is different from Parser.Default is confusing and something I think should be considered a bug (or at least a poor design decision) but is not directly relevant to how AutoHelp should behave except in as much that it just reveals that the current implementation has too many inconsistencies and it's therefore difficult to use it to establish the "desired" behaviour. I still think the actual behaviour of this PR (and #458) is sensible and desired. Can you explain where you think it isn't?

[x789]

@nemec commented on Mar 26, 2018

I think this solves the issues nicely. Can you add a few tests to help us prevent regressions? I think the >following should cover things, but if you think of any other scenarios feel free to include them:
AutoHelp = false and invoking --help returns an error.
AutoVersion = false and invoking --version returns an error.
AutoHelp = false, a flag for --help added manually by the user to the Options class, and it can be invoked successfully.
AutoVersion = false, a flag for --version added manually by the user to the Options class, and it can be invoked successfully.

The requirements stated above are correctly implemented in CommandLineParser v2.5.0. But in my (and others, see #294 and #414) opinion two important scenarios are missing:

• AutoHelp = false and there is no manually added verb/option called 'help'
• AutoVersion = false and there is no manually added verb/option called 'version'

I suspect that if these requirements had been considered in #256, it would have been noticed that the automatically generated HelpText does not match the parser configuration.

[NeilMacMullen]

two important scenarios are missing

Indeed, and worth adding there are now unit tests that explicitly check these scenarios.... see line 681 albeit together. In #458 I've broken the combined test into two individual ones for paranoia's sake, but the behaviour is the same. See line 678

[moh-hassan]

@NeilMacMullen
I use fiddle as a way of simple quick online demenstoration even with this small bug of redirection :)
For deep debug I go to Vs2017 or Linqpad.

My concerns is that the intention of the original issues #87 and #200 (disable help/version generation by building their own custom HelpText) which is fixed by Pr #256 are different than the intention of the issues #414 and #455 (which simply hide help/version option from auto generated help) and therefore should have different resolution.

BTW, --version and --help are LNX/POSIX standard in all commandLine utilities (this library respect and implement this standard ) and the requirement of the issues #414 and #455 for hiding them may violate this standard.
Are we ready to violate this standard?!

Any enhancement or Fluent interface API to HelpText is welcome and it's needed in #39 and can be completed with this PR and yours without Breaking Change or obsolete AutoBuild.
AutoBuild is the engine of generating help for Parser.Default.

[x789]

@moh-hassan

My concerns is that the intention of the original issues #87 and #200 (disable help/version generation by building their own custom HelpText) which is fixed by Pr #256 are different than the intention of the issues #414 and #455 (which simply hide help/version option from auto generated help) and therefore should have different resolution.

I think this is a misunderstanding. This pull request #461 has nothing to do with the configuration of HelpText. It simply hides 'help' and 'version' if these verbs/options are not understood by the current parser configuration. Nothing else.

BTW, --version and --help are LNX/POSIX standard in all commandLine utilities (this library respect and implement this standard ) and the requirement of the issues #414 and #455 for hiding them may violate this standard.
Are we ready to violate this standard?!

Unfortunately, CommandLineParser violates the POSIX standard by allowing multi-character options, double-dashes and verbs. Chapter 12.2 is very precise.

I wasn't aware that CommandLineParser 'enforces' the presence of help and version. If this restriction is really wanted, it should be documented in a prominent place.

[moh-hassan]

@x789
I was evaluating CommandLineParser against POSIX standard in this comment

The Readme link this Getopt ref

Can you give me an example of the violation of the multi-character options?

If this restriction is really wanted, it should be documented in a prominent place.

Yes, you are correct. Attention will be take for the documentation and I hope contribution in Wiki.

[moh-hassan]

@x789

two important scenarios are missing:
AutoHelp = false and there is no manually added verb/option called 'help'
AutoVersion = false and there is no manually added verb/option called 'version'

These two scenarios are described in the wiki in
Example1: Disabling AutoHelp
Example2: Disabling Version

In these scenarios, parser consider --help and --version an unknown option and raise an error
and consequently no help/version screen are generated.

Developers can handle these cases (if needed) in:

                 parserResult.WithNotParsed(ShowError);

and show their own custom help/version or custom error screen

[x789]

@moh-hassan
You type faster than I do :-). I hope to catch up...

I was evaluating CommandLineParser against POSIX standard in this comment

The Readme link this Getopt ref

Thank you for referring to the getopt-documentation. I can understand your point of view now.
However, I think that a 'long option' is against the POSIX standard. According to POSIX, an option may only be designated by a single character. (This is the first item on the list.) In contrast, the 'GNU option characteristics' allow multiple characters.

But for me the crucial question is whether CommandLineParser is based on the constraint that a program must always have 'help' and 'version'. If it does, then #414 and #455 are no bugs and the auto-generated HelpText is fine. I would then treat them as feature requests which I would reject with reference to that constraint. (see my comment below)

[x789]

With the knowledge gathered in this discussion, I have once again read through the complete documentation of the module.

@moh-hassan I can now completely understand your argumentation. Some points are hidden between the lines in the documentation. In my experience, however, it is normal for a developer documentation not to contain and explain every (implicit) decision made.

Here's my suggestion on how to proceed:

• This PR and Issues Hide "version" and "help" commands from display #294, Incorrect help text if AutoVersion = false #414 and Help Text Displays 'help' Option/Verb if AutoHelp is 'False' #455 are closed. This is because the automatisms in Parser assume that the verbs/options 'help' and 'version' are present. (Regardless of whether they are created by the user or by the parser.)
• @moh-hassan's new wiki page "Adding of implicit Help and Version screen and disabling auto generation" will be added to the official wiki.
  • I suggest that the first note be changed to "This feature control the generation of version/help verbs/options and it is not hiding help/version options in the auto generated help."
• The reference to GNU getopt is more prominent in the ReadMe. It is moved from "Resources for NewComers" to "At a Glance".
• We create a new feature request for HelpText.AutoBuild. This describes that you can specify whether 'help' or 'version' should be displayed. You can set this property for instances of HelpText, but not when calling AutoBuild().

[moh-hassan]

@x789
Thanks for your suggestions.
I'll convert your words into action plan.
The feature request for HelpText.AutoBuild to hide/show help/version option as a property in the HelpText is the exact solution.

[moh-hassan]

Closed based on @x789 in his comment

[Athari]

@moh-hassan

My concerns is that the intention of the original issues #87 and #200 (disable help/version generation by building their own custom HelpText) which is fixed by Pr #256 are different than the intention of the issues #414 and #455 (which simply hide help/version option from auto generated help) and therefore should have different resolution.

#414 and #455 don't look like "simply hiding" for me. Hiding an unavailable command is correct. Hiding an available command is a bug. So the main problem is that the pull request #256 didn't test generation of error message help properly.

@x789's suggestion of completely separating availability as a command and availability on the help screen sounds really weird to me, to be frank. Maybe I'm missing something or I'm unaware of technical limitations, but that's not what users of command line programs would ever expect.

BTW, --version and --help are LNX/POSIX standard in all commandLine utilities (this library respect and implement this standard ) and the requirement of the issues #414 and #455 for hiding them may violate this standard.
Are we ready to violate this standard?!

Removing "version" and "help" completely is bad, but I think it should be allowed. The library is not standards police.

These two scenarios are described in the wiki

I can't access wiki for some reason, GitHub keeps redirecting me to the front page of the fork.