CLI interface should be at the object, not class, level

[timothysmith0609]

Bug report

When attempting to implement #658 (repeatable flags), our test suite would break, since flags marked repeatable would persist, even beyond the boundaries of test methods (e.g. testB would contain parsed options from testA). Initially I thought this was an error in Thor, but on closer inspection, it appears to be an issue in the design of our CLI commands.

Our CLI commands all follow a similar invocation design:

def command
  rescue_and_exit do
    KLASS.from_options(options)
end

I propose changing KLASS.from_options(options) to KLASS.new(options). This gives us finer-grained control over the scope of these CLI invocations.

Expected behavior: [What you expected to happen]

krane_deploy("-f foo bar") -> 'filenames = ['foo', 'bar']
krane_deploy() -> "filenames = []

Actual behavior: [What actually happened]
krane_deploy("-f foo bar") -> 'filenames = ['foo', 'bar']
krane_deploy() -> "filenames = ['foo', 'bar']

Version(s) affected:
version 1 and up are affected, though currently we don't do anything that causes this behaviour to make problems for us.

Steps to Reproduce

set repeatable: true to any CLI arg and run the test suite

[KnVerey]

Given that the KLASS.from_options implementations should be (and seem to actually be?) stateless, I don't understand why changing them to instances could matter. Can you explain more about why you think this is the problem?

[tsontario]

Given that the KLASS.from_options implementations should be (and seem to actually be?) stateless, I don't understand why changing them to instances could matter.

It used to be stateless (well, it would only change state once after parsing via the @assigns instance var): see here.

However, when repeatable options were introduced, we changed the behaviour to this:

  def assign_result!(option, result)
    if option.repeatable && option.type == :hash
      (@assigns[option.human_name] ||= {}).merge!(result) # <-uh oh! @assigns may contain leftover information from a previous invocation!
    elsif option.repeatable
      (@assigns[option.human_name] ||= []) << result # same issue here
    else
      @assigns[option.human_name] = result
    end
  end

So instead of reassigning the value of @assigns[option_name], we're merging it when repeatable == true. Unfortunately, this appears to have broken an assumption in Thor. Namely that we'll never parse multiple commands for a single instantiation of Thor. Using instances instead of KLASS.from_options would get around this, since there is now more state being persisted. I'm not fully convinced that's the solution, though.

Since we only ever expect Options::parse to be called once per command, it might be more straightforward to simply reset @assigns at the beginning of ::parse (though it's a bit messy to ensure we're restoring the default values appropriately).

[KnVerey]

Making parse idempotent certainly sounds conceptually appealing. I'm a little concerned that if we work around this by using instances, it is only going to bite other Thor users in the future. Since everything in our own (krane) code is stateless, the side-effects feel quite surprising.

for a single instantiation of Thor

Can you clarify what you mean by this?

[tsontario]

Update: so of course after all of this investigation the fix is exactly 4 characters long

A bit more research has left me with more questions than answers.

Options::@default behaves strangely when required: false

I was/am still struggling to understand the exact mechanism whereby options were being persisted across separate command invocations. My hypothesis up to now has been that the @assigns instance variable is keeping old references because of the merging strategy introduced by the repeatable flag logic.

During inspection, however, I noticed something odd: the @default variable for the --filenames was also being mutated by the provided filenames. I then discovered that @default and the actual parsed values exist in the same object :(

(byebug) @assigns["filenames"].object_id
47447049247820
(byebug) @switches["--filenames"].default.object_id
47447049247820
(byebug) @assigns["filenames"].object_id
47447049247820
(byebug) @assigns["filenames"]
[]
(byebug) @switches["--filenames"].default
[]

... and on the subsequent invocation:

(byebug) @switches["--filenames"].default
[["/my/file/path", "foobar"]]
(byebug) @switches["--filenames"].default.object_id
47447049247820 # <- same object as above and mysteriously mutated

Maddeningly, I can't see where/how this is taking place. The docs do note that it is invalid to declare required: true AND declare a default. Setting required: true does in fact yield the correct behaviour for our use case (and since --stdin is deprecated, --filenames will eventually be required), but Thor is still broken for required: false && default != nil.
We actually can't set required: true because it would break existing code that depends exclusively on the --stdin flag, which is only deprecated, not retired.

for a single instantiation of Thor

Can you clarify what you mean by this?

By that I mean when Thor is loaded into memory. E.g. we load it once into memory for the test suite, but invoke it multiple times.

[timothysmith0609]

Fixed upstream by rails/thor#715