Add summary_width and summary_indent to OptionParser in Crystal
#15326
Conversation
spec/std/option_parser_spec.cr
Outdated
| # it "raises an error for invalid summary_width or summary_indent" do | ||
| # parser = OptionParser.new | ||
| # expect_raises ArgumentError, "Invalid summary width: -10" do | ||
| # parser.summary_width = -10 | ||
| # end | ||
| # expect_raises ArgumentError, "Invalid summary indent: -5" do | ||
| # parser.summary_indent = -5 | ||
| # end | ||
| # end |
There was a problem hiding this comment.
Why is this spec commented?
There was a problem hiding this comment.
Hi Thank you for your review!
These tests do not work as expected.
The first test fails because it does not verify that summary_width is greater than or equal to 0. Should we change summary_width to UInt32 ? Or should we keep the current code as is and instead add a method named summary_width= that raises an ArgumentError in such cases?
The second test fails to compile simply because the argument is a string. This test is probably unnecessary.
There was a problem hiding this comment.
Maybe keep the signed Int32 but silently clamp it to 0..?
There was a problem hiding this comment.
The type should be Int32. Clamping would be fine, but we could alternatively also raise ArgumentError.
Dunno which makes more sense here, but in doubt I'd prefer the more strict variant of raising.
There was a problem hiding this comment.
Thanks, I'll keep it signed Int32 and clamp it to 0.. silently
| description = description.gsub("\n", "\n#{indent}") | ||
| if flag.size >= 33 | ||
| @flags << " #{flag}\n#{indent}#{description}" | ||
| width = summary_width.clamp(0..) |
There was a problem hiding this comment.
I'd suggest raising on negative values instead.
There was a problem hiding this comment.
Like this?
if summary_width < 0
raise ArgumentError.new("Negative summary width: #{summary_width}")
end
# Replace width with summary_widthI am fine with either. I understand the idea that exceptions are more convenient. Here I followed Ruby's OptionParser as it seems to clamp.
Hi!
This pull request introduces enhancements to the
OptionParserin CrystalThese features are similar to the
summary_widthandsummary_indentfound in Ruby'sOptionParser.See #14153
Motivation
Currently, the indentation settings in
OptionParserare hard-coded, which can limit the flexibility and fun of creating command line tools. The default width is set to 32, the same as the Ruby default, which is too wide for many use cases. Being able to customize these settings would be beneficial for a variety of command line utilities.Implementation
With the help of ChatGPT and Gemni, I looked into how these features are provided in major programming languages, but the naming and APIs vary across languages and libraries, and best practices are unclear. So I simply went for the Ruby approach.
Actually, I don't really care about this approach or implementation. I hate the fact that you can't specify indentation in the standard library, so I submitted it because I thought no one would touch it unless I submitted a pull request.
I'm willing to close this if someone provides a better pull request.
Thank you.