[224] Add on_error block for options

[apohllo]

Implements:

opts = Slop.parse do |o|
  o.separator 'Check which definition questions does not have answers yet.'
  o.separator 'Options:'
  o.int '-o', '--option', 'some option', required: true
  o.on_error do puts o; exit; end
end

[apohllo]

Any update on the status of this pull request?

[leejarvis]

@apohllo Thanks for this, and apologies for the late reply. I took a quick look but I'm not a huge fan of passing the Slop instance into each option (you'll notice parts of the code are designed to circumvent this intentionally). I see why it's been done though and there's no obvious workaround, but I'd like to take a proper look at your code because I have any good suggestions. Hoping to do that over the weekend

[apohllo]

@leejarvis, me either. But making a proper implementation without it would require a major refactoring I believe. The error detection code would have to be moved to another class, with exactly this responsibility. That might be sensible, but I am not sure I'm the best person to implement it.

[leejarvis]

@apohllo Thanks. What if we store the error block into the config, and then push that config down into each of the options independently rather than the options instance itself? e.g.

class Options
  DEFAULT_CONFIG = {
    # ...
    on_error: ->(err) { raise err }
  }

  def on_error(&block)
    config[:on_error] = block
  end
end

class Option
  def ensure_call(value)
    # ...
    err = Slop::MissingArgument.new("missing argument for #{flag}", flags)
    on_error.call(err)
  end

  def on_error
    config.fetch(:on_error)
  end
end

This way we:

• Don't need to pass the parent options instance into each option instance
• Don't need to consider having a nil error hander. The default will just raise as it does right now
• Don't pass the Options instance into the callback. I don't think this is necessary since the options should be available in the wider scope
• Have the added benefit of consumers using the on_error config option if they don't want to use the method call. This would allow nice things like: Slop.new(on_error: ErrorHandler.new)

WDYT?

[apohllo]

Now it's beautiful IMHO :-) Almost additions only.

[leejarvis]

@apohllo Thanks for this, apologies for the delay. I'm generally happy with it and think it's almost ready to merge. I do have a comment about the test file though; I think we don't need to test all of the existing error cases, and instead just test 2 things:

1. That the existing behaviour is working as expected (maybe this is already covered by another test?)
2. That the new custom behaviour is working as expected (that is, we can pass our own custom on_error block

I think these tests can be added to the existing options_test.rb or option_test.rb depending on which class is being tested.

Also, could you have a bash at adding something to the README for this behaviour? I'm happy to take a look at it if you'd like.

[apohllo]

1. I can move the tests to already existing files. Yet I think that a test for each type of exception is a good idea - without all these tests, the implementation would be broken. They will serve as a good regression indicator. But if you think it's better to have fewer tests, I will remove them.
2. I will provide the documentation.

[leejarvis]

@apohllo Fair comment if those tests helped build your implementation. Happy to merge once docs are up to date. Thanks!

[apohllo]

Hi, any update on the PR?

[wooly]

Hi @leejarvis, can this one get merged?

[leejarvis]

@apohllo Any desire to pick this back up? No pressure, I can work on it

[apohllo]

I think all changes were done.