Allow leading hyphen in switch values when specified with =#737
Conversation
1e04cf3 to
214acdb
Compare
|
|
||
| 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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
OK - let's add some documentation for this in the option/options portion then.
There was a problem hiding this comment.
Can you clarify what you're referring to by "option/options portion"? Is this something I should be adding as part of this PR?
There was a problem hiding this comment.
My bad - I forgot what project I was talking about 😛 The update would be to the Wiki once this is merged.
|
|
||
| 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)) |
There was a problem hiding this comment.
OK - let's add some documentation for this in the option/options portion then.
| @switches = {} | ||
| @extra = [] | ||
| @stopped_parsing_after_extra_index = nil | ||
| @is_switch_value = false |
There was a problem hiding this comment.
This is actually incorrect, no? This is a value with a dash that is specifically not a switch.
There was a problem hiding this comment.
I'm not sure I understand. This instance variable is true if and only if the current element in the pile being processed is considered to be a value even if it would normally be considered an option (i.e. it has a leading -). The initial state is that nothing has been parsed yet, in which case we want to parse it as an option if it has a leading -, not as a value.
There was a problem hiding this comment.
Yep, exactly. So when this variable is true, the current argument is a value, not a switch. The name "is switch value" seems to say "the current value is actually a switch".
Maybe something like is_literal_value or something similar?
There was a problem hiding this comment.
I landed on is_treated_as_value. PTAL @dorner
dorner
left a comment
There was a problem hiding this comment.
Much better - looks good to me! @rafaelfranca ?
This is a fixed version of #498.
This allows
--foo=-barto work.The implementation works by keeping track of an additional piece of state
@force_current_to_be_value, which is maintained insideshiftandunshiftto ensure it matches the state in@pile.