Improve support for multiline help text

[NeilMacMullen]

This PR improves support for multiline and semi-formatted HelpText. For example, it is now possible to write

[Option(HelpText=
@"This option could be used in lots of ways:
        * a flag
        * another thing
or just  ignored altogether")]
public string AnOption {get;set;}

and have the help text output in the expected fashion. Sub-indentation is correctly preserved even when long lines are supplied. E.g.

"Use this like
- this
- or in another way that takes quite a lot of words to describe
- or like this"

will be output as

Use this like
- this
- or in another way
that takes quite a lot
of words to describe
- or like this"

for a narrow screen.

There are a couple of unit tests supplied to ensure correct operation (I'm not sure these adhere to the current naming convention - happy to change if you want).

As well as the main functionality there are a few 'drive-by' changes in the HelpText and test code.

• I've hoisted a few bare numbers into constants to improve consistency
• I refactored the main text-wrapping code since it was actually duplicated previously.
• Some of the original tests were actually asserting less-than-optimal operation. Specifically, they were asserting the requirement for trailing spaces at the end/beginning of lines and were also incorrectly asserting that lines would be wrapped at one character less than the allowed line width. It should be pretty obvious from the diffs which ones I've fixed and why.

Any questions, drop me a line. This is great library I've been using for a couple of years and it's good to see it back under active maintenance :-)

[NeilMacMullen]

Hmm , I notice that the tests are failing on the automated test system, and from the log it appears that the newline detection is not working. That is odd since the tests all pass on my (windows) PC and I've used Environment.NewLine rather than hardcoding assumption about linefeeds.

My guess would be that the tests are being compiled in a way that that the implicit line-feeds in HelpTextWithLineBreaks_Options are not being inserted as Environment.NewLine. Are the tests running under Linux?

[NeilMacMullen]

Tests now passing. I have added an extra test to ensure correctness when cross-compiling or when editor line-endings are not consistent with the target machine

[NeilMacMullen]

@moh-hassan any chance of a review/merge on this? I can't see that it's at all controversial though I suppose there's a small chance it might annoy people who have been inserting \n into their help text then manually trying to adjust the indentation.

[moh-hassan]

I start reviewing the PR.
As you use \r & \n and the library is used in windows/Linux/OS X, it's usually using Environment.NewLine because it's Platform Independent.

[moh-hassan]

To overcome the problem of line-break in test, the library break the help text into lines like:

        // Verify outcome
                    var lines = result.ToNotEmptyLines().TrimStringArray();
        #if !PLATFORM_DOTNET
                    lines[0].Should().StartWithEquivalent("CommandLine");
                    lines[1].Should().BeEquivalentTo("Copyright (c) 2005 - 2018 Giacomo Stelluti Scala & Contributors");
        #else
                    // Takes the name of the xUnit test program
                    lines[0].Should().StartWithEquivalent("testhost");
                    lines[1].Should().StartWithEquivalent("© Microsoft Corporation");
        #endif
                    lines[2].Should().BeEquivalentTo("ERROR(S):");
                    lines[3].Should().BeEquivalentTo("Option 'badoption' is unknown.");
        lines[4].Should().BeEquivalentTo("USAGE:");
         .....

for example

[NeilMacMullen]

@moh-hassan Thanks for looking at this. The line-break situation is more subtle than you might think (indeed, it took me a couple of rounds of broken tests to work out what was going on).
When the user writes

HelpText = @"abc
def"

It will be compiled to "abc\r\ndef" when

• compiled on a windows machine when files are checked out using windows line-endings
• compiled on a linux machine when files are checked out using windows line-endings
• compiled on a linuxmachine when files are checked out using linux line-endings but the user's editor is set to windows line-endings (and prior to checkin/checkout loop

There is also a set of scenarios that lead to the string being compiled as "abc\ndef"

Furthermore, the user might choose to write

HelpText ="abc\r\ndef"

even though the code ends up being compiled on a linux machine.

With .net core , we have cross-platform executables so there is no guarantee that the executable was compiled on a machine where Environment.NewLine matches the line-breaks embedded in the string.

Hence the code has to take special care to ensure that line-breaks in compile-time strings are treated flexibly while being rigorous about using Environment.NewLine for strings that are output to the user at runtime. That is why you see the processing of '\r', '\n' in the TextWrapper on input (constructor) and use of Environment.NewLine on output (ToText)

To overcome the problem of line-break in test, the library break the help text into lines like:

I'm not sure that's really relevant here. The main reason for breaking the output string in most of the tests appears to be because 1) it's easier to look at individual lines when asserting relevant content, 2) those tests aren't really concerned with the stuff at the end of the lines!

[moh-hassan]

The problem of line-break in Cross-Platform is solvable in GIT and CI like Appveyor by properly handling the line endings.
In Appveyor.yml, init section, you can find the command:

       git config --global core.autocrlf input

This core.autocrlf setting tells git to convert newlines to LF when checking out files.
So it's compiled and tested in one environment whatever the original files saved with \r\n or \n as a line-break

As, you use Environment.NewLine all-the time for Cross-Platform compatibility for a line-break in the library, in test, if you want to exclude line-break from string comparison:

instead of:

        lines = input.Replace("\r","");

use:

        lines =input.Replace(Environment.NewLine,"") ; 

[NeilMacMullen]

@moh-hassan Please could you reread my post. Environment.NewLine is a runtime variable and hence should not be used when processing compile-time strings.

If you want a concrete example you might think about what the output of

var verbatim=@"file-specific line
break";

Console.Writeline(verbatim.Replace(Environment.Newline,"-break-");

when compiled on Linux and run on Windows vs compiled on Windows and run on Linux.

Even if Environment.NewLine was not likely to change at runtime, it is possible for users of the library to embed the 'wrong' style of line-endings in a string based on (false) assumptions about the target platform....

var writtenByADevWhoThinksTheyAreOnLinux= "linux\nbreak";
var writtenByADevWhoThinksTheyAreOnWindows= "windows\r\nbreak";
 
Console.Writeline(writtenByADevWhoThinksTheyAreOnLinux.Replace(Environment.Newline,"-break-");
Console.Writeline(writtenByADevWhoThinksTheyAreOnWindows.Replace(Environment.Newline,"-break-");

Again, it's a useful experiment to consider what happens when you consider the output when compiled/run on each platform.

in test, if you want to exclude line-break from string comparison

You are misunderstanding the purpose of those tests. The intention is to ensure that the line-breaks are present in the expected positions, but to be agnostic about what style they are.

[moh-hassan]

Environment.NewLine is a runtime variable and hence should not be used when processing compile-time strings.

From MS Documentation:

Environment.NewLine Property

A string containing "\r\n" for non-Unix platforms, or a string containing "\n" for Unix platforms.
Remarks
The property value of NewLine is a constant customized specifically for the current platform and implementation of the .NET Framework.

What about this compile-time variable (NOT run-time):

         var verbatim=$"file-specific line{Environment.NewLine}break";
         Console.Writeline(verbatim.Replace(Environment.Newline,"-break-");

when compiled on Linux and Windows and then run on Linux and windows.

[NeilMacMullen]

What about this compile-time variable (NOT run-time):
var verbatim=$"file-specific line{Environment.NewLine}break";

That is not a compile time constant. It is an expression which is evaluated at run-time by appending the run-time value of Environment.Newline to the constant "File-specific line" and then appending the constant "break".

If you're doubtful about that then I suggest you try compiling this....
const string notACompileTimeConstant = "abc" + Environment.NewLine;

And if it was truly defined at compile-time, then how do you suppose a cross-platform .Net core application would produce the correct line-endings when you run the following?
Console.Write("a string"+Environment.NewLine);

The property value of NewLine is a constant customized specifically for the current platform and implementation of the .NET Framework.

This just means it is a constant for the platform you happen to be running on (as opposed to the platform you are compiling on).

BTW, I don't have visibility of your build system to say how it is misconfigured but I can assure you that this whole issue started because a unit test like this was failing....

var str =@"abc
def";
Assert(str == "abc"+Environment.NewLine +"def");

[moh-hassan]

When I meant Compile-time on the following statement:
var verbatim=$"file-specific line{Environment.NewLine}break";

I mean, $ string interpolation - is evaluated at compile-time not at run-time

My suggestions of the Environment.NewLine was based on the guideline from NetCore team when building NetCore application.
I'll continue to review the PR :)

[NeilMacMullen]

I mean, $ string interpolation - is evaluated at compile-time not at run-time

Ah - but that is exactly the wrong way round ! If string-interpolation was conducted at compile-time, then it couldn't possibly produce the right result for

Console.WriteLine($"hey - the date is {DateTime.Now}");

:-)

My suggestions of the Environment.NewLine was based on the guideline from NetCore team when building NetCore application

Indeed, this is generally very good guidance! But you need to be aware that it applies to strings produced or consumed at runtime (ie. supplied by or displayed to the user) not constant things! If you've ever worked with different dates or cultures then you can see this analogous to using int.Parse(..., InvariantCulture) for strings that are internal to the code and int.Parse(...,localCulture) for strings that are supplied by the user. :-)

[moh-hassan]

I added two extra test cases with the attached file:

1- For Hidden option
2- With MaximumDisplayWidth of small value.

But they fail

class1.zip

[NeilMacMullen]

Thanks! The first test failure is actually not a bug. The original indenter had a flaw/bug where it would add a space to every line except the last. The new indenter doesn't suffer from this problem. That's why you'll see I've modified some of the original tests to remove the check for the trailing space.

The second test really was a bug so thanks for catching this! I've modified the TextWrapper to always use a minimum width of 1 even if there is no space left on the display. Probably worth noting that the visual effect will be horrible in this case but it would also have been horrible using the old indenter! If you want to display helptext using 10 columns it's never going to look good ;-)

I noticed your "hidden option" test was actually present in the original test set but had been commented out (not by me) some time in the past. That led to find a load of other tests that seem to have been ignored at some point by someone who couldn't get them to run under .net core. I have now restored these tests by simply avoiding the breaking bit of the test - i.e. the assumption that the copyright line will include some specific text.

[moh-hassan]

"hidden option" test was actually present in the original test set but had been commented out (not by me) some time in the past

You are correct. 18 test case was dropped in moving from v2.3.0 to v2.4.3
I have restored these tests in PR #462 (not merged yet) with the modification of copyright line that include some specific text matching the dotnet test suit.
I did an integration test between you PR and PR462 and it pass successfully except the test of the hidden option.
so, I preferred to set it in a temporary separate test case until the final merge for ease invistagation.
You need not to double the effort of restoring these tests and PR462 cover it.

I've modified the TextWrapper to always use a minimum width

Setting a minimum width is a good choice.
Sure 10 columns is never going to look good and user should provide at least a minimum width to avoid the exception fired in the first test case, otherwise you may set the width to a minimum value or fire exception
The basecode fire an exception if the width <1.

The Refactoring done in your PR is nice.

[NeilMacMullen]

I have restored these tests in PR #462 (not merged yet)

Ok - I suggest that when you have merged #462 I will rebase this PR and resolve my local changes with the 'final' tests.

[moh-hassan]

I did an integration test in my local machine in QA branch that merge your PR and Pr #462 and Passed: 374 and only 1 Failed (good news that hidden options passed)

CommandLine.Tests.Unit.Text.HelpTextTests.Invoke_AutoBuild_for_Options_with_Usage_returns_appropriate_formatted_text [FAIL]
Failed CommandLine.Tests.Unit.Text.HelpTextTests.Invoke_AutoBuild_for_Options_with_Usage_returns_appropriate_formatted_text
Error Message:
Expected lines[3] to be equivalent to
" Token 'badtoken' is not recognized." with a length of 37, but
"Token 'badtoken' is not recognized." has a length of 35.
line 499

It seems the two space prefix

I can modify this test case and remove the extra spaces. Can I do?

[NeilMacMullen]

It seems the two space prefix

yes agreed - I think that test is so old it precedes the change that added indentation for errors and other items. Adding a couple of spaces or, better, using

var lines = text.ToNotEmptyLines().TrimStringArray();

resolves it.

[NeilMacMullen]

BTW - this PR is really just a precursor to adding a fix for #337 After you have merged it I would like to add a "Detail" parameter for HelpText which will be shown if you explicitly call "help" on a verb. The TextWrapper class is a useful building block for that.

[moh-hassan]

Now, the test case is resolved and test pass.

Can you have a look to the layout of the help for the test case of "hidden option" in the attached file that I sent before. I find an extra empty line for the option "input-file" and no line between options.

The layout of the help is:

CommandLine.Tests.dll 1.9.4.131

v, verbose This is the description of the verbosity to test out the
wrapping capabilities of the Help Text.
input-file This is a very long description of the Input File argument that

            gets passed in.  It should  be passed in as a string.

help Display more information on a specific command.
version Display version information.

[moh-hassan]

this PR is really just a precursor to adding a fix for #337

I expect so.
The enhancement could allow for "pre-options-summary-text" and "post-options-summary-text" blocks

[NeilMacMullen]

Can you have a look to the layout of the help for the test case of "hidden option" in the attached file that I sent before. I find an extra empty line for the option "input-file" and no line between options.

I believe this is an artefact of the fact that test uses AddOptions directly instead of HelpText.AutoBuild. Options are only separated with blank-lines when the HelpText.AdditionalNewLineAfterOption property is set to true and this is only done in one place - one of the HelpText.AutoBuild variants.

So in other words, I don't think it is an error that I have caused - just another indication that the current HelpText configuration is too complicated and confusing! ;-)

[NeilMacMullen]

I just pushed in another couple of tests to prove that blank lines are added by default between verbs on the main help screen and between options on a specific "help verb" screen.

[moh-hassan]

An integrated branch is recreated QA with merging this PR and merging fix/missed-test #462 and find 5 test cases Fail.
Result of CI Appveyor here

It seems that the extra spaces is the problem.
The test report is attached including help layout:
xunit-report.zip
You can revert the commit of "fix up old broken tests" because these changes are in PR #462

[moh-hassan]

I have resolved 3 test cases by removing one trailing space because it has no effect on logic of wrapping.
The rest 2 cases has no trailing spaces.
You can inspect these two tests in the attached file.
two-tests.zip

Also removing the trailing one space cause the success of test with this PR in the integration branch, it can't be applied to PR #462 because it cause test fail.
If possible, setting the front/trailing space as in the current v2.5.0 is preferred and keeping the tests in PR #462 ASIS without removing front/trailing space is the recommended.
In other words, refactoring code should insure success of the old tests.

[NeilMacMullen]

The test fails you are seeing are due to bugs in the original indent/wrap code which the new version does not share.

1. Because the original code incorrectly added a space when wrapping lines, it did not fully use the available column width. So in test When_help_text_is_longer_than_width_it_will_wrap_around_as_if_in_a_column_given_width_of_40 you can see that the string "...of the verbosity to test" actually does have a width of 40 and the old test was wrapping unnecessarily and incorrectly.

2. The old code could also add a preceding space when wrapping a string of the same length as the column width. That is why in test Long_pre_and_post_lines_without_spaces, the test incorrectly requires a space in " After" . Frankly this test should never have passed review since it's requiring output which is clearly incorrect.

The other test failures are variants of these.

In other words, refactoring code should insure success of the old tests.

Normally I would agree that changing unit tests is undesirable and the goal should be to maintain existing behaviour. However, in this case the refactoring exposed bugs in the original design and the tests had not only ignored those bugs, but baked them into the expected behaviour! I believe the new behavior is correct (though if you can find examples where it isn't I'll be happy to fix them) and therefore the tests should be corrected.

From a practical perspective, why not just treat 'develop' as the integration branch and start by merging #462? As I said, I'm quite happy to rebase to the new develop.

[moh-hassan]

I agree that adding space when wrapping lines isn't necessary, failure of these tests due that extra space can be corrected by modifying these tests.

why not just treat 'develop' as the integration branch

I prefer to keep the history of develop as clean as possible and merge when the PR is final and no more iteration of change. As you see there were more iterations of change in both Pr(s).
When start merging Pr #462 your PR will fail test. These tests can be modified in a separate commit in your PR.

I'll merge PR #462 in develop.

[moh-hassan]

Pr #462 is merged in develop.

[NeilMacMullen]

Thanks - have now rebased from develop so this should include tests from #462

[moh-hassan]

+1
Great. It's ready for merging.

[NeilMacMullen]

Thanks! :-)