Skip to content

Conversation

@kbilsted
Copy link
Contributor

When options or arguments are parsed into an enum, the help text will now include the values contained in the enum.

fixes #92

Copy link
Owner

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

This is a good start! I made a few suggestions below, but overall I like it. The approach here is simple and clean. Thanks!

I suspect this doesn't work on attributes? Have you tried it on a small program like this?

enum Color { Red, Blue }
class Program
{
  [Option]
  public Color Color { get; }
}

@natemcmaster
Copy link
Owner

Also, it looks like there are some test failures on Linux. It looks to me like the HangingIndentWriter is adding new lines, possibly because it is detecting Console.BufferWidth as a small number, or because it's failing altogether and defaulting to 80. Might be good to see if we can write a test what won't change its results based on the size of the console used to launch the test.

Relevant code to look at:

/// <summary>
/// Get the Console width.
/// </summary>
/// <returns>BufferWidth or the default.</returns>
private int? TryGetConsoleWidth()
{
try
{
return Console.BufferWidth;
}
catch (IOException)
{
// If there isn't a console - for instance in test enviornments
// An IOException will be thrown trying to get the Console.BufferWidth.
return null;
}
}

/// <summary>
/// The default console width used for wrapping if the width cannot be gotten from the Console.
/// </summary>
public const int DefaultConsoleWidth = 80;
private readonly bool _indentFirstLine;
private readonly int _indentSize;
private readonly int _maxLineLength;
private readonly string _paddedLine;
/// <summary>
/// A description formatter for dynamically wrapping the description to print in a CLI usage.
/// </summary>
/// <param name="indentSize">The indent size in spaces to use.</param>
/// <param name="maxLineLength">The max length an indented line can be.
/// Defaults to <see cref="DefaultConsoleWidth"/>.
/// </param>
/// <param name="indentFirstLine">If true, the first line of text will also be indented.</param>
public HangingIndentWriter(int indentSize, int? maxLineLength = null, bool indentFirstLine = false)

@kbilsted kbilsted force-pushed the pr/write_accepted_enum_values_in_help_text branch 3 times, most recently from d0d2506 to 322363d Compare September 16, 2019 19:16
@kbilsted
Copy link
Contributor Author

PR ready for review again

kbilsted and others added 10 commits September 19, 2019 22:39
When options or arguments are parsed into an enum, the help text will now include the values contained in the enum.

fixes natemcmaster#92
Used e.g. for testing.

Also made the default constructor public. This allows the DefaultHelpTextGenerator to be instantiated on a per-use basis in order to keep unit tests a isolated from each other as possible.
Without that change, all instances would be the same singleton instance - i.e. a test can affect another test by fiddling with the overridden width.
Even though a CommandLineApplication holds lists of `CommandArgument` and  `CommandOption` they are not used
in the default help text generation.
@kbilsted kbilsted force-pushed the pr/write_accepted_enum_values_in_help_text branch from 322363d to 18bb134 Compare September 19, 2019 20:40
Copy link
Owner

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

Thanks for updating! I had a just a few more comments to address about the new public APIs added. Once addressed, I’ll merge this in. I’m planning for this to be part of 2.5, planning to release in a few weeks.

@natemcmaster natemcmaster merged commit 30e486b into natemcmaster:master Sep 20, 2019
@natemcmaster natemcmaster mentioned this pull request Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display possible value for enum in Help text

2 participants