Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage text suggestions #19

Open
DannyBen opened this issue Nov 12, 2019 · 16 comments
Open

Usage text suggestions #19

DannyBen opened this issue Nov 12, 2019 · 16 comments

Comments

@DannyBen
Copy link

Hi,

I am playing with this nice gem shard, and noticed some things that I think could benefit from a slight change.

Consider this output:

  Commands:
    help [command]  # Help about any command.
    kill <pid>      # Kills server by pid.

  Flags:
        --cache-dir  # Caching directory. default: ''.
    -h, --help       # Help for this command. default: 'false'.
    -p, --port       # The port to bind to. default: '8080'.
        --verbose    # Enable verbose output. default: 'false'.

Suggestions:

  1. Remove the # symbol that separates the flag/command and its description. Having 2 or 3 spaces between the flag and the description will be cleaner and still clear.
  2. Whenever the flag is a boolean or an empty string, there is no need to show the default value.
  3. I would consider removing the single quotes around the default value - so default: 8080. instead of default: '8080'.
  4. When the flag requires a value, it is usually shown in the usage text - like this: -p, --port <port>
  5. When the flag description text is too long, I see there is no handling of wrapping the text to fit the screen width. For example - I wish that this output:
  Flags:
        --cache-dir  # Caching directory You can also enable this by using the environ
ment variable USE_CACHE. default: ''.
    -h, --help       # Help for this command. default: 'false'.

would look like this

  Flags:
        --cache-dir   Caching directory You can also enable this by using the
                      environment variable USE_CACHE. 
    -h, --help        Help for this command. 
@mrrooijen
Copy link
Owner

Thanks for the great input! Working on these changes now.

@mrrooijen
Copy link
Owner

One problem regarding having <port> in -p, --port <port>.

All flags have defaults, so no required option has been implemented. The reasoning behind it at the time was that I could infer the required type (String, Int, Float, Bool) from the default value. So there currently is no way to enforce setting a value, as it'll always just fall back to the default.

What about having -p, --port [port], since it's always optional. Or is that unconventional?

Note that eventually I want to see if I can do something about how parsed flags are returned. I'd love to be able to return a proper struct containing the values rather than using a hash/map. By that time I could also potentially tackle the issue of allowing a value to be marked as required. Haven't yet figured out how to deal with using structs, though. Might have to resort to the use of macros.

@mrrooijen
Copy link
Owner

Perhaps unnecessary to render -p, --port [port] since the default: 8080 is already present, which implies that it's optional.

@DannyBen
Copy link
Author

Well,

Using -p, --port [port] is not conventional and it implies that you can use --port without providing a port, which you should not be able to do.

In all command line applications that I see, the usage text clearly shows if the flag has an argument or not - since it helps to clarify the purpose. imagine this flag:

-c, --cache DIR

and compare it to how Commander displays it:

-c, --cache

In the first case, the user does not need to read the description to understand how it works. In the second case, they must.


Perhaps this can be solved easily, like how the docker command solves it - in case the flag is not a boolean type, they display the type in the usage pattern:

$ docker help build
... snip ...
  -m, --memory bytes            Memory limit
      --memory-swap bytes       Swap limit equal to memory plus swap:
                                '-1' to enable unlimited swap
      --network string          Set the networking mode for the RUN

... snip ...

Will that be easier / acceptable?

@mrrooijen
Copy link
Owner

There's no way to infer that you want a directory (DIR) as an argument to --cache (it'd infer <cache> which doesn't help, and wouldn't work if there was only a short flag (-c) and no long flag (--cache). You'd either have to rename the flag to --cache-dir, but then you'd still have an issue if you only wanted to provide a short flag (e.g. -c) and would have to use the description block to describe how to use the flag.

Perhaps it'd be better to allow you to specify DIR in long/short?

cmd.flags.add do |flag|
  flag.name = "cache"
  flag.short = "-c DIR"
  flag.long = "--cache DIR"
  flag.default = ".cache"
  flag.description = "Directory to place cache files."
end

@mrrooijen
Copy link
Owner

Note that DIR in short/long doesn't do anything. It's purely there to add additional information on what argument to provide to the flag and render that in the help screen.

@mrrooijen
Copy link
Owner

mrrooijen commented Nov 12, 2019

Alternatively, and much easier to implement and perhaps more understandable would be to allow you to specify a flag arg:

cmd.flags.add do |flag|
  flag.name = "cache"
  flag.short = "-c"
  flag.long = "--cache"
  flag.arg = "DIR"
  flag.default = ".cache"
  flag.description = "Directory to place cache files."
end

This way you don't repeat the same DIR arg in both flag.short and flag.long.

@DannyBen
Copy link
Author

Yes. Both options are nice.

In fact, the first option (flag.short = "-c DIR") is what I intuitively tried - but the second option is also very good - I am all for simplicity, so whatever is simpler is probably better.

@mrrooijen
Copy link
Owner

I opted for what you intuitively tried. It's more consistent with how you define a command, so to avoid inconsistencies we'll use the same notation style with flags. I ended up getting a simple implementation done for this regardless.

I haven't taken care of text wrapping because there doesn't appear to be a good way to determine the terminal width in Crystal at this time from what I can see. Though we might want to still wrap using at least a fixed width. Thoughts on this?

The individual commits:

These commits address points 1, 2, 3 and 4. You can try it in your application by referring to the help branch. Like so iirc:

dependencies:
  commander:
    github: mrrooijen/commander
    branch: help

You can specify the descriptive argument like so:

flag.short = "-c DIR"
flag.long = "--cache"

Or

flag.short = "-c"
flag.long = "--cache DIR"

Or

flag.short = "-c DIR"
flag.long = "--cache DIR"

All of the above are equivalent.

@DannyBen
Copy link
Author

Very nice. Just tested, and looks nice.

As for text wrapping, there are a couple of points to consider:

Newline between flag description and flag

In the command line frameworks I have built in Ruby (for example mister_bin) - I opted to always show the description of the subcommand or flag, indented and in a separate line than the flag. This alone makes most lines not wrap, since there is more space (here is an example)

For example:

  -h, --help
    Shows this help screen

This particular change becomes particularly crucial, when you have long flag names, imagine this:

  -x FOLDERS, --exclude-folders-regex FOLDERS

it does not leave you a lot of space for the description.
But - I know this is a matter of taste, and not necessarily something you want to do for Commander (of course, I am voting yes - it generates cleaner and easier to read descriptions, as you do not need to move your eyes to the right side of the screen to read descriptions).

Width detection

As for determining the width of the terminal - in Ruby, I have developed a gem that I am using successfully in all my CLI apps. One of its functions is to determine terminal width. Wouldn't this code work almost as is in Crystal?

Source

  def detect_terminal_size(default=[80,30])
    if (ENV['COLUMNS'] =~ /^\d+$/) && (ENV['LINES'] =~ /^\d+$/)
      result = [ENV['COLUMNS'].to_i, ENV['LINES'].to_i]
    elsif (RUBY_PLATFORM =~ /java/ || (!STDIN.tty? && ENV['TERM'])) && command_exist?('tput')
      result = [`tput cols 2>&1`.to_i, `tput lines 2>&1`.to_i]
    elsif STDIN.tty? && command_exist?('stty')
      result = `stty size 2>&1`.scan(/\d+/).map { |s| s.to_i }.reverse
    else
      result = default
    end
    result = default unless result[0].is_a? Integer and result[1].is_a? Integer and result[0] > 0 and result[1] > 0
    result
  end

Word wrapping

The same Colsole gem also has a word wrapping function I am using - perhaps it can be ported to Crystal as well?

Source

  def word_wrap(text, length=nil)
    length ||= terminal_width
    lead = text[/^\s*/]
    text.strip!
    length -= lead.length
    text.split("\n").collect! do |line|
      if line.length > length
        line.gsub!(/([^\s]{#{length}})([^\s$])/, "\\1 \\2")
        line.gsub(/(.{1,#{length}})(\s+|$)/, "#{lead}\\1\n").rstrip
      else
        "#{lead}#{line}"
      end
    end * "\n"
  end

Is any of this helpful?

@mrrooijen
Copy link
Owner

Nice. I wasn't aware of tput nor stty. I was aware of ENV["COLUMNS"] and ENV["LINES"] but these are apparently not always available. In any case we could at least provide text wrapping on a best-effort basis by attempting ENV, tput and stty where possible and if in the event that someone finds a situation where it doesn't work, we can see what other options are available (if any).

Putting descriptions on the next line also seems like a viable approach. Would that apply to both (sub)commands and flags, or just flags?

@DannyBen
Copy link
Author

In regards to width - yes, best effort is definitely the way to go. As you can see in my code, if everything fails, I am considering 80 to be the width.

As for new line in subcommands - dealers choice. In some of my implementations I did it like this, and in others, in the same line. I guess the important thing is that the usage looks as friendly as possible - usage for humans.

@mrrooijen
Copy link
Owner

Looking at docker help, they keep it all on a single line for both commands and flags, and the flags wrap, while the commands don't. I think I'm currently leaning towards keeping descriptions on the same line as the commands and flags, but letting the description text wrap for both if necessary. I'll have to experiment with it.

@DannyBen
Copy link
Author

By the way, another couple of things I am missing in all of Crystal's CLI frameworks that I have tested, and another reason I prefer to have all descriptions in a new line are these:

  1. I am missing the ability to describe what the args mean.
  2. I am missing the ability to describe examples.
  3. I am missing the convenient method to describe my subcommands in both a long and short form. Short form will be displayed when showing the general help, and long form when showing the help of the subcommand itself.

The below is an example output of a subcommand in one of my gems (which uses my mister_bin gem).

Points of interest:

  1. All text is aligned to the left without leading spaces - this allows me to simply use "\n" when I want to have a loner text (with commander, I need to use "\n " with two spaces, and hope these two spaces wont look differently in the future).
  2. Everything (commands, flags, parameters) show their description in a wrapped way, and starting at with newline.
  3. There is a special way for me to describe parameters.
  4. there is a special way for me to describe examples.

The DSL for the below output looks like this

$ radio set --help
Set the radio network and channel

This command provides both interactive and non-interactive means of storing the
currenlty active channel and network in the configuration file.

Usage:
  radio set [CHANNEL NETWORK]
  radio set --help

Options:
  -h --help
    Show this help

Parameters:
  CHANNEL
    AudioAddict channel key. You can use a partial key here or leave empty for
    an interactive prompt.
    Use '-' to set the first channel in the network.

  NETWORK
    AudioAddict network key. You can use a partial key here or leave empty for
    an interactive prompt.

Examples:
  radio set
  radio set punk
  radio set dance digitally
  radio set dance di
  radio set metal rockradio
  radio set - rockradio

@mrrooijen
Copy link
Owner

I've opened 3 new issues where these additions can be discussed. If you have any specific DSL/Output ideas for Commander for these specific features, submit them there.

@DannyBen
Copy link
Author

I've opened 3 new issues where these additions can be discussed

Good idea. This topic has become long :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants