Add support for repeatable options

[astratto]

If an option is marked as :repeatable => true, then its value is an array of values.

Hash

option :item, :type => :hash, :repeatable => true

$ mycmd --item name:one age:1 --item name:two age:2

options[:item] = [{"name" => "one", ...}, {"name" => "two", ...}]

Array

option :item, :type => :array, :repeatable => true

$ mycmd --item 1 2 3 --item 4 5 6

options[:item] = [["1", "2", "3"], ["4", "5", "6"]]

Note: This patch is backward-compatible, because options must be marked explicitly with :repeatable.

[astratto]

Hey guys, any feedback on this?

[astratto]

Just to give more context, I've pushed some documentation on my wiki's fork: https://github.com/astratto/thor/wiki/Method-Options

[sferik]

I’m curious what @wycats thinks of this feature.

[withnale]

Bump. This will be really useful... Any update on this?

[astratto]

@sferik I assume @wycats is pretty busy with other stuff...

Is there anything we can do to push this through?

[danleyden]

I'd really like to see this merged as well - any possibility of pushing it soon?

[sferik]

From your example, this code:

option :item, :type => :array, :repeatable => true

With this command:

$ mycmd --item 1 2 3 --item 4 5 6

Produces this option:

options[:item] = [[1, 2, 3], [4, 5, 6]]

First, it’s not obvious to me how this would create an array of integers. Where is the integer type declared? Normally, any input arriving via ARGV is a string, so I would expect the result to be:

options[:item] = [["1", "2", "3"], ["4", "5", "6"]]

It’s also not clear to me why specifying an array option multiple times would create an array of arrays, instead of appending to the array. I think some users would expect this result:

options[:item] = ["1", "2", "3", "4", "5", "6"]

Again, from your example, this code:

option :item, :type => :hash, :repeatable => true

With this command:

$ mycmd --item name:one age:1 --item name:two age:2

Produces this option:

options[:item] = [{"name" => "one", "age" => "1"}, {"name" => "two", "age" => "2"}]

This is confusing to me because the result is an array, not a hash. It seems odd that making an option repeatable would change the type of the option itself. Also, users might expect repeating an option would merge the hashes.

It would be helpful to understand your specific use-case for this feature so I can get a better understand your requirements. From that, we can work on a solution with a clear and simple interface.

[astratto]

Hi @sferik, thank you for your reply.

I'll try to reply to your comments and then I'll give you some use cases.

1. mycmd --item 1 2 3 --item 4 5 6:
   I fixed my comment, it returns arrays of strings.
2. appending vs arrays of arrays: the latter behaviour is consistent, assuming that :repeatable returns arrays of something. If we tried to append, what's the expected behaviour with your hash example? What key is going to be used?

From my point of view, if I specify an option twice I expect to get a set (array) of something and then I can manage it as I wish; if we introduced a magic merging strategy that would be tricky.

As far as use cases are concerned, I usually need two cases but I can probably imagine others:

• repeating an option would increase the level of a flag: this is pretty common, think of mycmd -vvv to set a verbosity level; you can achieve a similar result with mycmd -v 3, but it's just not what a good chunk of existing CLIs behave
• providing sets of structured data: e.g. mycmd --override X Y Z --override J K L; a possible example is shown in first my comment that allows to create N instances of a given item

Let me know if something's not clear.
Thank you again.

[sferik]

appending vs arrays of arrays: the latter behaviour is consistent, assuming that :repeatable returns arrays of something.

Right. But it’s inconsistent for a type: hash option to return an array when :repeatable is true.

If we tried to append, what's the expected behaviour with your hash example? What key is going to be used?

I’m not arguing that append or merge is necessarily the right behavior. I just don’t think it’s obvious what the behavior would or should be.

repeating an option would increase the level of a flag: this is pretty common, think of mycmd -vvv to set a verbosity level; you can achieve a similar result with mycmd -v 3, but it's just not what a good chunk of existing CLIs behave

I can think of a few different ways of supporting this feature without adding :repeatable. For example, we could make a change to how the string type works, returning the way it was specified instead of its name. For example:

  desc :test, "test"
  option :verbose, aliases: :v
  def test
    puts options[:verbose]
  end

Today, if this command was run as:

command test -vvv

It would output:

verbose

Instead of always setting the value of the option to its name, it could return the way the option was passed, in this case:

vvv

The downside of this approach is that it could be a breaking change, but hopefully no one relies on those values, since they are not so meaningful.

I still don’t fully understand your other use case but I would argue that providing complex structured data as command line arguments is probably not the best way to get that data into your program. Instead, consider allowing the user to specify that input from a file or stdin.

[astratto]

Right. But it’s inconsistent for a type: hash option to return an array when :repeatable is true.

I understand your point of view on that, but I think 'repeatable' reasonably implies that it's going to be a collection.

I can think of a few different ways of supporting this feature without adding :repeatable. For example, we could make a change to how the string type works, returning the way it was specified instead of its name.

That's an interesting idea, but that wouldn't help with mycmd -v -v -v and that's another quite common pattern.

I still don’t fully understand your other use case but I would argue that providing complex structured data as command line arguments is probably not the best way to get that data into your program. Instead, consider allowing the user to specify that input from a file or stdin.

That use case is probably a bit borderline and I tend to agree that it's not an ideal user experience; but when the target of that binary is a script things are slightly different.

Think of a simpler use case --append value1 --append value2.

Honestly, I don't care too much about the implementation details, but I think such a feature is really useful. What do you think?

[danleyden]

I'd still really like to see this feature, whatever the internal implementation is.

I write command-line tools and regularly have need for things like @astratto is suggesting. Whatever the internal representation ends up looking like after implementation, it would be really useful to be able to specify commands that can be used by scripts like:

mycommand --arg value1 --arg value2

Without that, I have to write really ugly workarounds that my users need to know about and understand like: mycommand --arg.1 value1 --arg.2 value2 (then I need to know how many to support as the maximum) or mycommand --arg=value1,value2,value3 then post-process them outside thor. This means that my users have to learn this alien syntax which is completely different to the syntax of other commands they use on their linux machines and macs making it harder for them to use the tools I write...

[astratto]

@sferik I'm sorry I keep pushing, but I'd like to understand what's your (as in Thor maintainers) take on the concept of allowing repeatable options.

Right now I'm not talking about the specific implementation, but I think we can close this issue if you're completely against the whole idea.

[rafaelfranca]

I like the idea and I think the implementation make sense.

$ mycmd --item 1 2 3 --item 4 5 6

generates

options[:item] = [["1", "2", "3"], ["4", "5", "6"]]

and not

options[:item] = ["1", "2", "3", "4", "5", "6"]

because if I provided the option twice I wanted two different values and not only one. If I want the later I can do mycmd --item 1 2 3 4 5 6.

Same with the hash case.

The only problem is, when the option is repeatable, should mycmd --item 1 2 3 4 5 6 return options[:item] = ["1", "2", "3", "4", "5", "6"] or options[:item] = [["1", "2", "3", "4", "5", "6"]]?

[astratto]

Let's close this before it turns 3...

[jamlevi]

Years later here, I have a need for a repeatable Hash option. And I find the current implementation does not provide what I am looking for, while the proposal in this closed PR does, for the Hash case. To illustrate, looking at the current test at https://github.com/rails/thor/blob/master/spec/parser/options_spec.rb#L420, why would this all be munged together into 1 hash? The other repeatable collection type, Array, yields an Array of Arrays, so why does hash type not yield an Array of Hashes?

Perhaps, I am looking at the use case differently than the authors of the current implementation?

For example, person_provisioner --person name:bob age:32 gender:m --person name:amy age:23 gender:f --person name:sue age:31 gender:f should yield an Array of Hashes like [{name:bob, age:32, gender:m},{name:amy, age:23, gender:f},{name:sue, age:33, gender:f}]. no?

Yet, it currently yields 1 person Hash: "person"=>{"name"=>"sue", "age"=>"33", "gender"=>"f"}, the last one passed in.

So, I applied @astratto's code for repeatable hash locally and it works for my use case, yielding an Array of Hashes, as passed in.

diff --git a/lib/thor/parser/options.rb b/lib/thor/parser/options.rb
index 8c1bbe7..ba16ffe 100644
--- a/lib/thor/parser/options.rb
+++ b/lib/thor/parser/options.rb
@@ -113,7 +113,12 @@ class Thor
             switch = normalize_switch(switch)
             option = switch_option(switch)
             result = parse_peek(switch, option)
-            assign_result!(option, result)
+            if option.repeatable
+              (@assigns[option.human_name] ||= []) << result
+            else
+              @assigns[option.human_name] = result
+            end
           elsif @stop_on_unknown
             @parsing_options = false
             @extra << shifted

diff --git a/spec/parser/options_spec.rb b/spec/parser/options_spec.rb
index b1e50fb..999e18c 100644
--- a/spec/parser/options_spec.rb
+++ b/spec/parser/options_spec.rb
@@ -415,9 +415,15 @@ describe Thor::Options do
         expect { parse("--attributes", "name:string", "name:integer") }.to raise_error(Thor::MalformattedArgumentError, "You can't specify 'name' more than once in option '--attributes'; got name:string and name:integer")
       end
 
+      # it "allows multiple values if repeatable is specified" do
+      #   create :attributes => Thor::Option.new("attributes", :type => :hash, :repeatable => true)
+      #   expect(parse("--attributes", "name:one", "foo:1", "--attributes", "name:two", "bar:2")["attributes"]).to eq({"name"=>"two", "foo"=>"1", "bar" => "2"})
+      # end
+
       it "allows multiple values if repeatable is specified" do
         create :attributes => Thor::Option.new("attributes", :type => :hash, :repeatable => true)
-        expect(parse("--attributes", "name:one", "foo:1", "--attributes", "name:two", "bar:2")["attributes"]).to eq({"name"=>"two", "foo"=>"1", "bar" => "2"})
+        expect(parse("--attributes", "name:one", "age:1", "--attributes", "name:two", "age:2")["attributes"]).to eq([{"name"=>"one", "age"=>"1"},
+                                                                                                                     {"name"=>"two", "age"=>"2"}])
       end
     end

So, not sure if you want to Re-Raise another PR or not? I would like to see the above get merged, but perhaps there was a good reason for the current implementation and people are using it as is? Thanks @astratto

[dorner]

@jamlevi it looks like there's a specific case in that implementation that merges hashes. Unfortunately we can't change that behavior without breaking it and everyone who relies on it.

We'd need to either create a new type of option or a brand new type (hash_array?) to enable this behavior.

[jamlevi]

@dorner Thanks very much, yes, after going over the comments again, specifically, #468 (comment), it's a bit more clear to me: "providing sets of structured data" was NOT implemented in master. I'll just go with my patched version for my use case, but maybe there are other people who would like to a "hash_array", as you suggest? Bow to all you maintainers.