crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.33k stars 1.61k forks source link

OptionParser Rewrite #8656

Open shinzlet opened 4 years ago

shinzlet commented 4 years ago

I think that OptionParser in its current state is lacking flexibility, and I want to discuss ways we could improve upon it. It is excellent for very small CLI programs, but many CLIs cannot be entirely implemented with it.

Take the familiar crystal CLI, for example. OptionParser natively only supports dash-prefixed flags, such as -f or --long-flag. This means that a command like crystal run [...] is not natively supported with OptionParser. In fact, the crystal CLI skirts this with a case statement, identifying each subcommand keyword manually (like play, build, or tool), then invoking OptionParser on the remaining options given that context.

Another issue is that, with OptionParser, short flag commands cannot be stacked - that term may be unfamiliar, but I'm referring to commands like pacman -Syu, where each letter, S, y, and u, are separate flags.

I wrote a library to address these issues, as well as several other ones I have encountered. It supports deep nesting of subcommands, stacking single-letter flags, and many other features (like optional fuzzy matching). I think that it could serve useful as a reference for code style or implementation details.

Here is a style example for a partial nmcli imitation (there are more examples in my library repo):

require "phreak"

Phreak.parse! do |root|
  root.bind(word: "wifi", description: "Configure or control wireless connections.") do |wifi|
    wifi.banner = "Manage wireless connections. Invocation: cli wifi [subcommand]"

    wifi.bind(word: "status", description: "Get the current status of the modem.") do
      # Reponds to "nmcli wifi status"
    end

    wifi.bind(word: "set", description: "Set the the wifi state.") do |set|
      # Responds to "nmcli wifi set"
    end

    wifi.bind(word: "help",
              description: "display specific help about the `wifi` subcommand.") do |help|
      puts wifi
    end
  end

  root.bind(word: "help", description: "display this help menu.") do |help|
    puts root
  end
end

I'd love to see what you think about revamping OptionParser to enable these sorts of behaviors. Also, I'd like to thank @RX14 for her kind words, and her suggestion to propose a rewrite of OptionParser.

RX14 commented 4 years ago

OptionParser isn't currently suited to building the complex commandline tools that users expect today without workarounds like the ones used in the compiler. Many option parsing libraries have sprung up to try and combat that, many using macros, or other complex DSLs.

The most interesting thing about @shinzlet's library to me is that it's API is very similar to the existing OptionParser API. This means OptionParser can incrementally be improved, while supporting the old API (perhaps with some deprecations). I'd like not to pass this opportunity up.

asterite commented 4 years ago

The compiler is using OptionParser just fine. It just pops the first word as a subcommand, even trying to match a prefix of it.

So saying OptionParser isn't suited to building complex command lines when it's already being used in the compiler seems a bit counter-intuitive.

RX14 commented 4 years ago

@asterite it can be done but i think the way it's currently done is a bit of a hack and can be improved a lot.

ryanprior commented 4 years ago

FWIW here's my experience with OptionParser and phreak. I decided to build a bunch of CLIs as part of my 2019 Advent of Code, with the intention to solve each puzzle using a nice well-behaved tool written in Crystal. I chose initially to use OptionParser in order to get familiar with it and to use standard Crystal features where available. But after implementing a few, and needing to go back and add more functionality to old ones as requirements piled up, I felt that OptionParser was fighting me at every turn and I had to write and refactor a bunch of boilerplate every time I wanted to update my interface.

After a few days I switched to use mosop/cli instead, which is excellent and allowed me to easily write and maintain a variety of CLIs without ever feeling I was fighting against the API or obfuscating the intent of my code. I tried a rewrite in phreak first but didn't like the pattern of nested blocks with constant calls to .bind. Since I didn't finish my rewrite using phreak I can't comment on the polish or maintainability, but I can say that my mosops/cli based tools were easy to polish and to maintain while adding new features and options.

shinzlet commented 4 years ago

@ryanprior I appreciate the feedback! I agree with some points you made, and I think mosop deserves lots of credit for the thought he put into that tool. I think the main benefits are that options are written on new lines (because they're just variable declarations in a class), and there are some brilliant features like the any_of selector.

I personally still like the semantic association with using a bind method, rather than the ambiguous meaning of making each command a nested class. Ultimately, a cli is a way of pattern matching textual inputs to function calls, which feels more like a binding than a class declaration to me.

A nice middle ground could be making the bind method slightly less verbose, and instead passing a configuration object to the block. For example:

# How Phreak does things currently
root.bind(short_flag: 'f', long_flag: "foo", description: "foo bar baz qux") do |sub|
end

# New proposal
root.bind(short_flag: 'f', long_flag: "foo") do |sub, options|
  options.description = "foo bar baz qux"
end

This would allow horizontal character count to be traded for an increase in line count, which I'm in favor of. It would also enable more expandability in the future - an "any_of" predicate much like mosop's could be implemented without bloating the bind method signature more.

ryanprior commented 4 years ago

Thank you @shinzlet for your thoughtful reply!

The "options as a sub-class" pattern definitely isn't what I like about @mosop's solution (tagging in since we love your work!) - it works fine, and doesn't particularly offend me, but imho it's kind of a code smell to use classes for something that really has nothing to do with OO design.

What I like most is that, classes or no, it's straightforward to declare your intent. A common-case flag or argument is a short one-liner that does exactly what you want, and a more flexible declaration that takes a block is available for when you want to do something unusual (like increase the verbosity level for each extra -v)

Let me try to compare a common case in your new proposal to cli:

record CliOpts, verbose : Bool, quiet : Bool, input : String
cli_opts = CliOpts.new(verbose: false, quiet: false, input: "-")
root.bind(short_flag: 'v', long_flag: "verbose", do |sub, options|
  options.description = "print verbose output"
  cli_opts.verbose = true
end
root.bind(short_flag: 'q', long_flag: "quiet", do |sub, options|
  options.description = "suppress most output"
  cli_opts.quiet = true
end
root.bind(short_flag: 'i', long_flag: "input", do |sub, options|
  options.description = "read input from file (default stdin)"
  # not sure how to read the next token as an argument in Phreak? do I just .shift off something? this isn't covered in the docs as far as I can tell?
end
puts "starting task" if cli_opts.verbose
# do work etc etc

versus cli might be something like:

class Program < Cli::Command
  class Options
    bool ["-v", "--verbose"], desc: "print verbose input", default: false
    bool ["-q", "--quiet"], desc: "suppress most output", default: false
    string ["-i", "--input"], desc: "read input from file (default stdin)", default: "-"
  end
  def run
    puts "starting task" if options.verbose?
    # etc do work
  end
end
Program.run ARGV

the crucial thing is not that it's nested tidily into classes, it's that this is a common case like most CLIs I whip up, and the intent is clear as can be in the cli paradigm. if I were to rewrite the API to not use classes and be kinda in-between phreak and CLI, I might try an API something like

options = OptionParser.bind(ARGV) do |opt|
  opt.bool ["-v", "--verbose"], description: "print verbose input", default: false
  opt.bool ["-q", "--quiet"], description: "suppress most output", default: false
  opt.string ["-i FILE", "--input FILE",
             description: "read input from FILE (default stdin)",
             default: "-"
end
puts "starting task" if options.verbose?
# do some work &c

This avoids a bunch of class messiness, and gives us functions we can compose together rather than being tempted to do fancy class inheritance things, and I would love to see an API like that. Maybe now that I've written this speculative code sample I should take a crack at implementing that API? Any thoughts welcome.

RX14 commented 4 years ago

I think it wouldn't be hard to build a DSL like the one above on top of OptionParser, but it would hard to go the other way around.

In the stdlib, we favour more powerful primitives, even if they're uglier, so that others can specialize them and make them prettier for individual cases.