Slop::Options and Slop::Parser not decoupled

[moritzschepp]

Excellent library, I use it a lot. Thanks for sharing it!

This is not really a bug. I just didn't expect the behavior. Consider this:

o = Slop::Options.new
o.string '--something', 'give me something'
opts1 = Slop::Parser.new(o).parse([])
opts2 = Slop::Parser.new(o).parse(['--something', 'cool'])

It seems that now

opts1[:something] == 'cool'

I'm using slop 4.6.2.

[leejarvis]

Thanks for reporting. This is interesting, and definitely unexpected behaviour. I would consider it a bug of the internal API.

I suspect it's happening because Slop#option is mutated at parse time. Since your example only has a single set of options, the first #parse mutates the option, and it's not reset in the second parse call. Freezing the option demonstrates this:

o = Slop::Options.new
o.string '--something', 'give me something'
o.options.first.freeze
opts1 = Slop::Parser.new(o).parse([])
opts2 = Slop::Parser.new(o).parse(['--something', 'cool'])

#=> /lib/slop/option.rb:41:in `reset': can't modify frozen Slop::StringOption (FrozenError)

Ideally we would be able to deal with this gracefully, but it may be a bit of a stretch goal, and I'm not sure it's worth spending much time on this if it's only used internally (note that Options encapsulates its own parser. Perhaps we should only ever allow a single parser<=>options combination. I'll have a think about this

[moritzschepp]

Thanks for your quick reply!

I just had a look at parser.rb and options.rb. It seems that the parsing actually happens in the Slop::Options class. The parser does reset the options instances before each parse but then they are filled with the results of the parse run. I agree it would be a lot of effort to change that. The only (ugly) way I can imagine is something like this:

class Slop::Parser
  def initialize(options, **config)
    @options = options.dup
    @options.instance_variable_set('@options', @options.map{|o| o.dup})
    @config = config
    reset
  end
end

But I guess this is not what you meant with "graceful" :)

Otherwise, a simple workaround is to regenerate the Slop::Options instance for every Slop::Parser.

Perhaps mentioning this in the README can also help.

[leejarvis]

Yeah, I think we might be able to create something (only slightly) nicer using initialize_copy. I think dup'ing the options object on the Parser is probably a good idea, so I'll have a look into doing that