Skip to content

Allow leading hyphen in switch values when specified with = #737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 8, 2021
Merged

Allow leading hyphen in switch values when specified with = #737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
214acdb
Allow leading hyphen in switch values when specified with =
univerio Sep 2, 2020
dc3f45d
force_current_to_be_value => is_switch_value
univerio Jun 5, 2021
7d1bd79
is_switch_value => is_treated_as_value; force_value => is_value
univerio Jun 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion lib/thor/parser/options.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def initialize(hash_options = {}, defaults = {}, stop_on_unknown = false, disabl
@switches = {}
@extra = []
@stopped_parsing_after_extra_index = nil
@is_treated_as_value = false

options.each do |option|
@switches[option.switch_name] = option
Expand Down Expand Up @@ -74,8 +75,19 @@ def peek
end
end

def shift
@is_treated_as_value = false
super
end

def unshift(arg, is_value: false)
@is_treated_as_value = is_value
super(arg)
end

def parse(args) # rubocop:disable MethodLength
@pile = args.dup
@is_treated_as_value = false
@parsing_options = true

while peek
Expand All @@ -88,7 +100,10 @@ def parse(args) # rubocop:disable MethodLength
when SHORT_SQ_RE
unshift($1.split("").map { |f| "-#{f}" })
next
when EQ_RE, SHORT_NUM
when EQ_RE
unshift($2, is_value: true)
switch = $1
when SHORT_NUM
unshift($2)
switch = $1
when LONG_RE, SHORT_RE
Expand Down Expand Up @@ -148,6 +163,7 @@ def assign_result!(option, result)
# Two booleans are returned. The first is true if the current value
# starts with a hyphen; the second is true if it is a registered switch.
def current_is_switch?
return [false, false] if @is_treated_as_value
case peek
when LONG_RE, SHORT_RE, EQ_RE, SHORT_NUM
[true, switch?($1)]
Expand All @@ -159,6 +175,7 @@ def current_is_switch?
end

def current_is_switch_formatted?
return false if @is_treated_as_value
case peek
when LONG_RE, SHORT_RE, EQ_RE, SHORT_NUM, SHORT_SQ_RE
true
Expand All @@ -168,6 +185,7 @@ def current_is_switch_formatted?
end

def current_is_value?
return true if @is_treated_as_value
peek && (!parsing_options? || super)
end

Expand Down
4 changes: 4 additions & 0 deletions spec/parser/options_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ def remaining
expect(parse("-f=12")["foo"]).to eq("12")
expect(parse("--foo=12")["foo"]).to eq("12")
expect(parse("--foo=bar=baz")["foo"]).to eq("bar=baz")
expect(parse("--foo=-bar")["foo"]).to eq("-bar")
expect(parse("--foo=-bar -baz")["foo"]).to eq("-bar -baz")
end

it "must accept underscores switch=value assignment" do
Expand Down Expand Up @@ -398,6 +400,7 @@ def remaining

it "accepts a switch=<value> assignment" do
expect(parse("--attributes=name:string", "age:integer")["attributes"]).to eq("name" => "string", "age" => "integer")
expect(parse("--attributes=-name:string", "age:integer", "--gender:string")["attributes"]).to eq("-name" => "string", "age" => "integer")
end

it "accepts a switch <value> assignment" do
Expand Down Expand Up @@ -425,6 +428,7 @@ def remaining

it "accepts a switch=<value> assignment" do
expect(parse("--attributes=a", "b", "c")["attributes"]).to eq(%w(a b c))
expect(parse("--attributes=-a", "b", "-c")["attributes"]).to eq(%w(-a b))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems confusing... if "-a" and "-c" are valid values for an array attribute, then why would both "-a b" and "-a b -c" give you two values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative is more confusing IMO and breaks backwards compatibility, i.e. --attributes=a b -c parsing to {"attributes" => ['a', 'b', '-c']} means that you can no longer have any options at all after an array option, which is not what the user expects.

This is the same reason this PR is limiting its scope to require a = to trigger this "leading - is a valid value" behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I think we'd need to document this specifically then.

There seems to be another PR to fix the same issue? #498 What's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this aims to fix the same issue as #498, but #498 appears to have been abandoned. I believe it also has a bug as described by this comment by @rafaelfranca:

I think setting this state here will broke it if there are more options after the first with - in the value, can you add tests to make sure it will work?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - let's add some documentation for this in the option/options portion then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what you're referring to by "option/options portion"? Is this something I should be adding as part of this PR?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad - I forgot what project I was talking about 😛 The update would be to the Wiki once this is merged.

end

it "accepts a switch <value> assignment" do
Expand Down