issue #482: reorder options in auto help text #484

b3b00 · 2019-07-23T06:44:23Z

this PR allows to redefine order using a Comparison function.
I choose to create a new ComparableOption struct from the Specification class to not break existing visibility rules (Specification is internal).
To redefine order you then have to provide a Comparison object as a static property of HelpText class before calling AutoBuild :

HelpText.OptionComparison = HelpText.RequiredThenAlphaComparison;
var helpMessage = HelpText.AutoBuild(parseResult,...)

Maybe not the perfect solution but it works well.
PR comes with the order that @tehkyle proposed.
When no comparison is provided before calling AutoBuild then it defaults to the current behavior (declaration order)

Unit test is also provided

Maybe a better and more elegant solution would be to provide a fluent API to configure HelpText AutoBuild.

Something like

HelpText.With(parseResult)
                .OnError((err) => { })
                .OnExample(example) => { })
                .WithOrder((ComparableOption attr1, ComparableOption attr2) => 1)
                .Build();

@maintaners what do you think ? do you prefer I rework it with fluent API or can we stay with it ?

This reverts commit 1811ce4.

b3b00 · 2019-07-23T12:28:59Z

I 've finally moved to a fluent API design as I found static properties are hell.
I kept the existing AutoBuild method not to break existing code.

var message = HelpText.CreateWith(parseResult)
                .WithComparison(HelpText.RequiredThenAlphaComparison)
                .OnError(error => {
                    throw new InvalidOperationException($"help text build failed. {error.ToString()}");
                })
                .OnExample(ex =>
                    {
                        return null;
                    })
                .Build();

moh-hassan · 2019-07-23T15:27:38Z

Thanks Olivier Duhart for your effort.
Your PR cover two valuable features: Reorder options and Fluent Interface.
It's preferred that every feature has its own branch/PR so every PR pass its own test locally and CI server then merged.
Can you move the Fluent Interface feature to a separate branch/PR and let this PR for Reorder options only and pass CI checks.

b3b00 · 2019-07-23T15:36:54Z

@moh-hassan , CI fails on a sensless unit test. So wouldnt it be easier to just remove this unit test and let CI succeed ?
I tried to order option on

required option
alphabetical order on option short name

but implicit options (--help and --version) does not have short name and then appeared before "real" options with my naive implementation of the comparison function. My bad I did not even try my UT before pushing it ! shame on me.

I have a local version of the UT that is OK : I 've simply modify the comparison function to take account of this particular edge case.

moh-hassan · 2019-07-23T16:09:31Z

The fail at line 186
It seems that you test the ordering using the new syntax Fluent Interface (FI).
Try to do your unit test of ordering options using the standard AutoBuild method and Isolate FI.
That is why it's necessary to move the Fluent Interface feature to other PR with its own unit tests.

moh-hassan · 2019-07-23T16:16:33Z

Good that you discovered the fail reason.
Test can cover: no-short-name/no-long-name/no-short-and-no-long-name/both-short-and-long-name.

moh-hassan · 2019-07-23T16:23:46Z

you can benefit from NameText : Gets a formatted text with unified name information.

b3b00 · 2019-07-24T06:17:09Z

@moh-hassan concerning unit tests, as order is defined using a custom Comparison function, I simply test with 3 cases :

order by required then alpha longName as asked by @tehkyle in issue.
order by required then alpha shortName : this one for testing comparison overriding
no comparison : this one to that behavior defaults to previous behavior (ordered by declaration order)

More tests are not that usefull I guess

moh-hassan · 2019-07-24T08:09:48Z

@olivier
HelpText fluent API shouldn't be part of this PR and I suggest moving it to other PR.

This reverts commit 113a21c.

b3b00 · 2019-07-24T08:43:14Z

I 've reverted all fluent API code. I 'll come back with the fluent API enhancement later if PR accepted and merged in develop.
So the use case is now :

Comparison<ComparableOption> myComparison= (ComparableOption attr1, ComparableOption attr2) => {
// some sensible comparison code in here
return 1; }
string message = HelpText.AutoBuild(
    parseResult, 
    error => {return null;},
    ex => { return null;},
    comparison:comparison
);

moh-hassan · 2019-07-24T18:54:41Z

src/CommandLine/Text/HelpText.cs

+        {
+            OptionSpecification option = spec as OptionSpecification;
+            ValueSpecification value = spec as ValueSpecification;
+            bool required = option != null ? option.Required : false;


Simplify to:

bool required = option?.Required ?? false;

moh-hassan · 2019-07-24T18:58:45Z

src/CommandLine/Text/HelpText.cs

+
+        public Comparison<ComparableOption> OptionComparison = null;
+
+        public static Comparison<ComparableOption> RequiredThenAlphaComparison  = (ComparableOption attr1, ComparableOption attr2) =>


Remove redundant else and braces, and use StringComparison.Ordinal

public static Comparison<ComparableOption> RequiredThenAlphaComparison = (attr1, attr2) => { if (attr1.IsOption && attr2.IsOption) { if (attr1.Required && !attr2.Required) return -1; if (!attr1.Required && attr2.Required) return 1; int t = string.Compare(attr1.LongName, attr2.LongName, StringComparison.Ordinal); return t; } return attr1.IsOption && attr2.IsValue ? -1 : 1; };

moh-hassan · 2019-07-24T19:04:43Z

src/CommandLine/Text/HelpText.cs

    public class HelpText
    {
+
+


#region Option ordering

moh-hassan · 2019-07-24T19:05:35Z

src/CommandLine/Text/HelpText.cs

+                return 1;
+            }
+        };
+


moh-hassan · 2019-07-24T19:25:42Z

tests/CommandLine.Tests/Unit/Issue482Tests.cs

+                .WithNotParsed(errors => { throw new InvalidOperationException("Must be parsed."); })
+                .WithParsed(args => {; });
+
+            Comparison<ComparableOption> orderOnShortName = (ComparableOption attr1, ComparableOption attr2) =>


Remove redundant else and braces, and use StringComparison.Ordinal as done here

moh-hassan · 2019-07-24T19:33:03Z

It looks fine.

You add a new parameter with default null to AutoBuild method. That is working.
To minimize the parameters passed to AutoBuild method, What about if it's set as a public property like other setting in HelpText :
```
public Comparison<ComparableOption> Comparison {get;set;}
```
Provide test case with some properties as value options one required and the other Value properties not required.
For documentation, Can you provide a wiki page describing how to to use the Order option feature with examples.

b3b00 · 2019-07-25T06:15:05Z

@moh-hassan
Regarding your remark on the Comparison value as an instance attribute : I agree there is to much parameters.
The autobuild method build the HelpText instance so we have to give it the optional comparison object if we want it to be used.
At the end of the method HelpText object is built AND helpmessage computed.
I don't see any simple method to remove parameters still keeping all the possibilities.
that was one of the purpose of my fluent API proposal : make it clearer to build an HelpText and compute message.

About your test case asking : my test case is the one you describe

2 required options
3 not required options
1 value

Could you give an example of a test case, please ?

About the wiki, I will do it when all dev done and checked, and when time available .

moh-hassan · 2019-07-25T19:27:15Z

All help configuration is supposed to be done using error Action, but this configuration is effective only (due to a bug in HelpText) when the parser fire parsing errors and it's not effective when using "--help".
This bug is resolved in the PR #467 (it's not merged yet) and now all configuration can be done using error Action
I merged your PR with PR #467 locally and did POC to use an instance variable instead of a parameter and it's working fine.

     var   message = HelpText.AutoBuild(parseResult, error =>
        {
            error.OptionComparison = HelpText.RequiredThenAlphaComparison;                
	return error;				
        },
        ex =>ex );

I can merge PR #467 to develop.
What are your suggestions?

b3b00 · 2019-07-26T06:03:07Z

@moh-hassan I would be easier if you merge #467 to develop, then I will rebase my branch with develop and do the fix according to #467 fix.
It look more appealing than the way I've done it : too many parameters I agree.
Let me know if you agree and do the merge.

thanks

Olivier

moh-hassan · 2019-07-26T08:38:00Z

It's merged.
Now test case can be:

     [Fact]
    public void AutoBuild_with_ordering()
    {      
        var parser = Parser.Default;
        var parseResult = parser.ParseArguments<Options_HelpText_Ordering_Verb1, Options_HelpText_Ordering_Verb2>(
                new[] { "verb1", "--help" }) //call help
            .WithNotParsed(errors => { ; })
            .WithParsed(args => {; });         

       //AutoBuild using instance variable without passing comparison  parameter 
        string message = HelpText.AutoBuild(parseResult, 
            error => 
			{
				 error.OptionComparison = HelpText.RequiredThenAlphaComparison;  
				return error;
			},
            ex => ex);
               
		var helps = message.ToString().ToNotEmptyLines().Skip(2).ToList();
       var expected = new List<string>()
        {
            "  -a, --alpha      Required.",
            "  -b, --alpha2     Required.",
            "  -c, --bravo",
            "  -d, --charlie",
            "-e, --echo",
            "-f, --foxtrot",
            "--help           Display this help screen.",
            "--version        Display version information.",
            "value pos. 0"
        };
        Assert.Equal(expected.Count, helps.Count);
        int i = 0;
        foreach (var expect in expected)
        {
            Assert.Equal(expect.Trim(), helps[i].Trim());
            i++;
        }           
    }

* develop: Add errs.Output extension method to auto direct help/version to Console.Out and errors to Console.Error Simplify if-else clause move IsHelp/Isversion to separate file with scope public Fix issue #259 for HelpText.AutoBuild configuration

moh-hassan · 2019-07-26T09:05:43Z

src/CommandLine/Text/HelpText.cs

        /// <remarks>The parameter <paramref name="verbsIndex"/> is not ontly a metter of formatting, it controls whether to handle verbs or options.</remarks>
        public static HelpText AutoBuild<T>(
            ParserResult<T> parserResult,
            Func<HelpText, HelpText> onError,
            Func<Example, Example> onExample,
            bool verbsIndex = false,
-            int maxDisplayWidth = DefaultMaximumLength)
+            int maxDisplayWidth = DefaultMaximumLength,
+            Comparison<ComparableOption> comparison = null)


//remove this line, no more parameters are used
//Comparison comparison = null

working on it.

moh-hassan · 2019-07-26T09:06:47Z

src/CommandLine/Text/HelpText.cs

        {
            var auto = new HelpText
            {
                Heading = HeadingInfo.Empty,
                Copyright = CopyrightInfo.Empty,
                AdditionalNewLineAfterOption = true,
                AddDashesToOption = !verbsIndex,
-                MaximumDisplayWidth = maxDisplayWidth
+                MaximumDisplayWidth = maxDisplayWidth,
+                OptionComparison = comparison


//remove this line, no more parameters are used
// OptionComparison = comparison

moh-hassan · 2019-07-26T09:07:46Z

tests/CommandLine.Tests/Unit/Issue482Tests.cs

+                    return error;
+                },
+                ex => ex,
+                comparison: comparison);


//remove this line
//comparison: comparison

moh-hassan · 2019-07-26T09:10:12Z

tests/CommandLine.Tests/Unit/Issue482Tests.cs

+                    ex => ex,
+                        false,
+                        80,
+                        orderOnShortName);


//remove the line orderOnShortName
//Enough to call without extra parameters:

string message = HelpText.AutoBuild(parseResult, error => { error.OptionComparison = orderOnShortName; return error; }, ex => ex);

b3b00 · 2019-07-26T09:30:48Z

I have trouble with onerror action. I sync my fork with upstream , I see your commandline#439 merge but it seems it does not call onerror when parse is ok.
I look at the AutoBuild code and indeed if parse is ok it does not call onerror. did something go wrong when merging ? could you check ?

moh-hassan · 2019-07-26T09:46:50Z

indeed if parse is ok it does not call onerror

That is correct.
onerror is called when parser fire parsing error or call '--help'
You need to modify the test cases and call help as given below ( my example show the use case of help and it is passed)

             var parseResult = parser.ParseArguments<Options_HelpText_Ordering_Verb1,    Options_HelpText_Ordering_Verb2>(
            new[] { "verb1", "--help" }) //call help
        .WithNotParsed(errors => { ; })
        .WithParsed(args => {; });

Note: This is the official way to test help: either by passing args '--help' or parser errors. It's not tested on parser success.
Also, you can use ToNotEmptyLines() method instead of splitting args using \r \n.

b3b00 · 2019-07-26T09:48:20Z

ok I didnot see the --help in test case. I correct it right now

b3b00 · 2019-07-29T07:44:24Z

@moh-hassan last build seems to have failed on upload failure. I can not restart and it does not seem to relate to my PR. Could you check it and maybe restart the build ?

moh-hassan · 2019-07-29T13:07:53Z

I think the problem is temporary and it's related to CI appveyor.
The parameter OptionComparison in AutoBuild isn't removed here. Can you update the commit fix and remove the parameter?

b3b00 · 2019-07-29T13:17:22Z

@moh-hassan : code seems ok, maybe you are looking at a stale version.

moh-hassan · 2019-07-29T13:56:15Z

I looked at your last commit fix (cff9fd1) for file changed here and here.

Can you point me where can I look for the last update?

moh-hassan · 2019-07-29T15:08:25Z

+1
Thanks Olivier Duhart for fixing.

b3b00 · 2019-07-29T15:14:40Z

@moh-hassan thank you for merging . Glad to have help a bit this project.

b3b00 added 6 commits July 22, 2019 13:37

issue #482 : fix

1811ce4

Revert "issue #482 : fix"

11beeca

This reverts commit 1811ce4.

#482 rework

f8d26bc

merge develop

9a9d27a

HelpText fluent API, initial

113a21c

fluent API for help message options ordering

6fc506d

cleaning

04a9bcf

correct UT

db7c20a

Revert "HelpText fluent API, initial"

a1ff100

This reverts commit 113a21c.

moh-hassan reviewed Jul 24, 2019

View reviewed changes

src/CommandLine/Text/HelpText.cs

public class HelpText

{

Copy link

Collaborator

moh-hassan Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#region Option ordering

moh-hassan reviewed Jul 24, 2019

View reviewed changes

src/CommandLine/Text/HelpText.cs

return 1;

}

};

Copy link

Collaborator

moh-hassan Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#endregion

moh-hassan reviewed Jul 24, 2019

View reviewed changes

correction after Pull-Request remarks

727f10c

set comparison on error action

59db0cd

moh-hassan reviewed Jul 26, 2019

View reviewed changes

fix

cff9fd1

cleaning

11c3a6e

moh-hassan merged commit f956dd7 into commandlineparser:develop Jul 29, 2019

JiajunW mentioned this pull request Aug 11, 2019

Clean up for #484 #500

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #482: reorder options in auto help text #484

issue #482: reorder options in auto help text #484

b3b00 commented Jul 23, 2019 •

edited

Loading

b3b00 commented Jul 23, 2019

moh-hassan commented Jul 23, 2019

b3b00 commented Jul 23, 2019 •

edited

Loading

moh-hassan commented Jul 23, 2019

moh-hassan commented Jul 23, 2019

moh-hassan commented Jul 23, 2019

b3b00 commented Jul 24, 2019

moh-hassan commented Jul 24, 2019

b3b00 commented Jul 24, 2019 •

edited

Loading

moh-hassan Jul 24, 2019

moh-hassan Jul 24, 2019

moh-hassan Jul 24, 2019

moh-hassan Jul 24, 2019

moh-hassan Jul 24, 2019

moh-hassan commented Jul 24, 2019

b3b00 commented Jul 25, 2019 •

edited

Loading

moh-hassan commented Jul 25, 2019

b3b00 commented Jul 26, 2019

moh-hassan commented Jul 26, 2019 •

edited

Loading

moh-hassan Jul 26, 2019

b3b00 Jul 26, 2019

moh-hassan Jul 26, 2019

moh-hassan Jul 26, 2019

moh-hassan Jul 26, 2019 •

edited

Loading

b3b00 commented Jul 26, 2019

moh-hassan commented Jul 26, 2019

b3b00 commented Jul 26, 2019

b3b00 commented Jul 29, 2019

moh-hassan commented Jul 29, 2019 •

edited

Loading

b3b00 commented Jul 29, 2019

moh-hassan commented Jul 29, 2019

moh-hassan commented Jul 29, 2019

b3b00 commented Jul 29, 2019


		public Comparison<ComparableOption> OptionComparison = null;

		public static Comparison<ComparableOption> RequiredThenAlphaComparison = (ComparableOption attr1, ComparableOption attr2) =>

issue #482: reorder options in auto help text #484

issue #482: reorder options in auto help text #484

Conversation

b3b00 commented Jul 23, 2019 • edited Loading

b3b00 commented Jul 23, 2019

moh-hassan commented Jul 23, 2019

b3b00 commented Jul 23, 2019 • edited Loading

moh-hassan commented Jul 23, 2019

moh-hassan commented Jul 23, 2019

moh-hassan commented Jul 23, 2019

b3b00 commented Jul 24, 2019

moh-hassan commented Jul 24, 2019

b3b00 commented Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moh-hassan commented Jul 24, 2019

b3b00 commented Jul 25, 2019 • edited Loading

moh-hassan commented Jul 25, 2019

b3b00 commented Jul 26, 2019

moh-hassan commented Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moh-hassan Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

b3b00 commented Jul 26, 2019

moh-hassan commented Jul 26, 2019

b3b00 commented Jul 26, 2019

b3b00 commented Jul 29, 2019

moh-hassan commented Jul 29, 2019 • edited Loading

b3b00 commented Jul 29, 2019

moh-hassan commented Jul 29, 2019

moh-hassan commented Jul 29, 2019

b3b00 commented Jul 29, 2019

b3b00 commented Jul 23, 2019 •

edited

Loading

b3b00 commented Jul 23, 2019 •

edited

Loading

b3b00 commented Jul 24, 2019 •

edited

Loading

b3b00 commented Jul 25, 2019 •

edited

Loading

moh-hassan commented Jul 26, 2019 •

edited

Loading

moh-hassan Jul 26, 2019 •

edited

Loading

moh-hassan commented Jul 29, 2019 •

edited

Loading