spectreconsole / spectre.console

A .NET library that makes it easier to create beautiful console applications.
https://spectreconsole.net
MIT License
9.47k stars 500 forks source link

Proposal: automatically display default values of options in the help page #973

Closed 0xced closed 1 year ago

0xced commented 2 years ago

Is your feature request related to a problem? Please describe. This is not a problem but a proposal to improve the help page of CLI commands.

Describe the solution you'd like Spectre.Console.Cli supports the DefaultValueAttribute to specify a default value for a command option. It would be nice to use this information to automatically display default values in the help page.

Describe alternatives you've considered The current alternative is to repeat the default value in the option description text. This could easily lead to desynchronized documented default value and actual default value. Also, a new column would make the default values stand out.

Additional context Here's what I quickly prototyped. I'll submit a proper pull request if this idea is appealing to you.

Running the Demo example with run --help before: image

Running the Demo example with run --help after: image

phil-scott-78 commented 2 years ago

I like it, especially if the behavior could be opt-in. I'd be tempted to see what it looks like with default value in-line with with the help text myself e.g. for -c the text "The default is Debug." is appended rather than needing another column of text.

0xced commented 2 years ago

Here's how it would look like:

Personally, I prefer the version with the additional DEFAULT column.

I'll have a look at how we could make this behaviour opt-in (or opt-out).

0xced commented 2 years ago

I think I nailed the opt-out part (could easily be modified to opt-in):

var app = new CommandApp();
app.Configure(config =>
{
    config.DisableOptionsDefaultValues();
});

See https://github.com/0xced/spectre.console/commit/05c2fe52922d88243409484afeb65396881d8e0b

FrankRay78 commented 1 year ago

Hi @phil-scott-78 - at the moment the PR is coded to show the default values in the help by default (excuse the pun). This strictly speaking doesn't break functional backward compatibility, however may cause problems in the (unlikely?) event consumers are somehow consuming the help output eg. in the same way Spectre uses Verify. However, they do have the ability to turn off this feature by config.DisableOptionsDefaultValues(); to revert to existing behaviour. Do you have a view on this?

FrankRay78 commented 1 year ago

Hi @0xced, firstly what a cracking idea and thank you for implementing it. I really like it and would be keen to incorporate it into the codebase.

However, (apologies), I've been doing some edge case testing and with very long default values eg:

public class CatSettings : MammalSettings
{
    [CommandOption("--agility <VALUE>")]
    [TypeConverter(typeof(CatAgilityConverter))]
    [DefaultValue(int.MinValue)]
    [Description("The agility between 0 and 100.")]
    [PositiveNumberValidator("Agility cannot be negative.")]
    public int Agility { get; set; }

    // I ADDED THE FOLLOWING ---->
    [CommandOption("--description <VALUE>")]
    [DefaultValue("Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum id diam at lacus vehicula lacinia pharetra et ante. Fusce ut neque sit amet lacus posuere aliquam et non dui. Vivamus nec leo volutpat mi pretium gravida rutrum sed lacus. In scelerisque turpis erat, non efficitur augue vestibulum ut. Vestibulum euismod urna id euismod pretium. Vestibulum laoreet congue tortor tempus pharetra. Nam in luctus erat. Quisque sollicitudin gravida imperdiet.")]
    [Description("A very long value.")]
    public string Description { get; set; }
}

The console wrapping messes up the alignment of the help text for the other commands, see:

image

BUT it does appear to cope well with fairly long (my judgement here, on behalf of the users) defaults, see:

image

Any thoughts? I guess we could....

  1. Do nothing about it - buyer beware
  2. Have the display of default values turned off by default - the user needs to explicitly turn it on
  3. Truncate it perhaps eg "Lorem ipsum dolor sit amet, consectetur adipiscing elit. [500 more characters follow]"
  4. Have an option somewhere so the user can toggle the truncate on/off
  5. Have an option called something like "HideLongDefaultValuesInHelp", defaulted to true
  6. Wrap the full default value IN the default column, AND ALSO, wrap the description IN the description column (if need be) - see image below

image

Any other ideas?

0xced commented 1 year ago
  1. Do nothing about it - buyer beware

Why not! (See also my replies to 2. and 6.)

  1. Have the display of default values turned off by default - the user needs to explicitly turn it on

I think having it on by default is better for discoverability of the feature. It's easier to search how to turn off the feature if you don't like it than learning about a nice feature you have no idea could exist.

  1. Truncate it perhaps eg "Lorem ipsum dolor sit amet, consectetur adipiscing elit. [500 more characters follow]"

The example you gave here is really pathologically long. In real life scenarios, I don't think people would design CLI options with such long default values. I have tried this feature on a few projects and the longest default value I came up with was a connection string, so something along those lines:

Data Source=myServerAddress;Initial Catalog=myDataBase;Integrated Security=SSPI;TrustServerCertificate=true

Also, truncating could be bad as it could hide crucial information (like in the connection string example).

  1. Have an option somewhere so the user can toggle the truncate on/off

I think this is overkill.

  1. Have an option called something like "HideLongDefaultValuesInHelp", defaulted to true

I also think this is overkill and would make the feature too much convoluted.

  1. Wrap the full default value IN the default column, AND ALSO, wrap the description IN the description column (if need be) - see image below

It would be really nice as it would solve all the problems related to long default values in a very elegant way. Now, that's where it becomes really interesting as it's actually already working, at least on macOS! I tried to add a very long Lorem ipsum default value in the Demo sample project and here's how it renders, with perfectly wrapped columns (builtin Terminal app at the top and iTerm at the bottom).

So the question becomes: why does text in a grid wraps fine on macOS but doesn't properly wrap in the Windows command prompt?

Wrapped columns in help text

Also, I have just rebased the help-options-default-value onto main and adapted a few things to be compatible with the recently merged #953.

phil-scott-78 commented 1 year ago

I think this is some good stuff as is. I'm gonna side with @0xced here, I think we could spend a ton of time trying to keep devs from shooting themselves in the foot with outrageous data but with this being something they are hard coding it isn't like something an end user would be breaking.

I feel like this is in good shape. I want to pull it down later tonight after my little guy goes to bed and I'll merge it in after my quick run through.

Long term @patriksvensson and I have discussed making a custom Help Writer something that could be plugged in. That way users wouldn't need a million levers and buttons to toggle to get things to their liking - we could provide output that should meet most use cases but those that want to get into the weeds could use their own implementation.

FrankRay78 commented 1 year ago

I'm in agreement too @0xced and @phil-scott-78 - the OOTB value here is great and should be turned on by default. The windows terminal wrap edge case can be raised as a separate issue for someone to look into and isn't big enough to hold up the merge.

My only comment is, given existing expectations have been updated to include the DEFAULT column, should we now add one test to include the output for when the user has opted-out of showing a default?

Excellent work.

0xced commented 1 year ago

should we now add one test to include the output for when the user has opted-out of showing a default?

Good point! I have added a test covering this scenario in 47cd07027445e57e6eb53c2b303c973a82c6e826.

FrankRay78 commented 1 year ago

FYI. Looks like the text wrapping not respecting the table column also affects Linux: (Ubuntu 22.04, .Net SDK 7.0)

image

(I will raise this as a separate issue to fix, once the code for this issue has been merged)


PS. Also interesting how the colours don't show either, eg. grid.AddRow(" ", "[lime]DEFAULT[/]", " "); there is no lime on either windows or linux terminals.

PPS. Even when I turn off all column wrapping:

image

Windows still wraps:

image

So there is something odd happening here, well beyond the scope of the change made in this issue.