-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
OptionParser Rewrite #8656
Comments
The most interesting thing about @shinzlet's library to me is that it's API is very similar to the existing |
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. |
@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. |
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 |
@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 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 # 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 |
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 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. |
I think it wouldn't be hard to build a DSL like the one above on top of 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. |
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 likecrystal run [...]
is not natively supported with OptionParser. In fact, the crystal CLI skirts this with a case statement, identifying each subcommand keyword manually (likeplay
,build
, ortool
), 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
, andu
, 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):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.
The text was updated successfully, but these errors were encountered: