Add getters for option in InvalidOption and MissingOption classes

[kojix2]

Hi all
When an error occurs in OptionParser, I would like to be able to catch which option caused the error.
Could it be changed this way?

[Blacksmoke16]

Related to #9264, I'd say this sounds reasonable.

[Sija]

option -> @option

ditto below

[kojix2]

Thank you.
Should we specify the String type?
getter option : String

[devnote-dev]

I'm not sure I understand the changes here: you can already access the invalid/missing option via the invalid_option and missing_option handler methods respectively, what need is there to have this on the exception classes too?

[kojix2]

@devnote-dev
Currently, I don't think there is a good way to catch which option caused the error.
For example, git commit -m.
I personally think it would be useful to display “Please write a commit message” and exit(1).

No, this could be accomplished with missing_option as you say.

[devnote-dev]

This is more a limitation of OptionParser, as far as I'm aware it doesn't parse options contextually so your only options (pun intended) are to do additional input checks in the missing_option handler, or of course use a dedicated shard.

[kojix2]

Rust has been successful in the area of command line tools. In my opinion, Crystal is also suited for this purpose. Don't give up on improving the standard library OptionParser...

[devnote-dev]

I wholeheartedly agree! Cling was built to support this. However the overall design of OptionParser is a different topic that spans multiple issues (#8656 should be a good place to start). I can't see how this PR would be helping the situation.

[kojix2]

OptionParser is designed in an object-oriented manner, but it tries to behave in a completely transparent way. This might be the right strategy during the early stages of development because setting constraints can reduce bugs and guide users in the right direction. However, I think sticking to this approach until the end may limit its flexibility. I’m not sure which stage OptionParser is at now, but I believe this is an important point to consider for future improvements.

This might seem unrelated at first, but it is actually closely connected to this pull request.

When trying to use invalid_option or missing_option, you often end up writing some logic inside the parser itself. For example, take a look at the sample code from the official API reference:

OptionParser.parse do |parser|
  parser.banner = "Usage: salute [arguments]"
  parser.on("-u", "--upcase", "Upcases the salute") { upcase = true }
  parser.on("-t NAME", "--to=NAME", "Specifies the name to salute") { |name| destination = name }
  parser.on("-h", "--help", "Show this help") do
    puts parser
    exit
  end
  parser.invalid_option do |flag|
    STDERR.puts "ERROR: #{flag} is not a valid option."
    STDERR.puts parser
    exit(1)
  end
end

In this example, OptionParser is responsible for handling a major task: exiting the program when an invalid option is detected. But should OptionParser really be responsible for such a task? Personally, I think it would be better if it simply reported the error and left the decision to terminate the program to another component.

I believe it would be more flexible if the parser ran once, and then another class handled any errors or decided whether the program should exit.

Separating option parsing from post-parsing actions — this is what I want. This pull request is a small step in that direction.

This text was translated from Japanese to English by ChatGPT 4o.

[devnote-dev]

In this example, OptionParser is responsible for handling a major task: exiting the program when an invalid option is detected. But should OptionParser really be responsible for such a task? Personally, I think it would be better if it simply reported the error and left the decision to terminate the program to another component.

Keep in mind that exit is a top-level method not a method of OptionParser, but I'm not sure how this relates to the PR. You're also not forced to handle this logic in the method handler block, there are many alternative ways.

I believe it would be more flexible if the parser ran once, and then another class handled any errors or decided whether the program should exit.

The handler methods exist because of OptionParser's design: everything is method-based which would have to include exceptions. The exceptions themselves only exist for this purpose as a default handler (and even then they're not all that necessary). What you're suggesting sounds like something for one of the discussions about OptionParser but does not relate to this PR.

[kojix2]

I wondered why you think that this is not related.
In Cling, the parser and the action runner are integrated. I know that this is a difficult design for Crystal's OptionParser. Asterite, the founder of Crystal, strictly requires that OptionParser behave transparently. (#9009)
In that case, I personally prefer to separate the parser and the runner, especially when there are subcommands. Parse all arguments and save the options and actions in an external object, and later trigger actions in another runner class based on the result.