Skip to content

🌈 More accurate Arguments#current_is_value? check#691

Merged
rafaelfranca merged 2 commits into
rails:masterfrom
timothysmith0609:looser_flag_detection
Nov 22, 2019
Merged

🌈 More accurate Arguments#current_is_value? check#691
rafaelfranca merged 2 commits into
rails:masterfrom
timothysmith0609:looser_flag_detection

Conversation

@timothysmith0609

@timothysmith0609 timothysmith0609 commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

Motivation

For a number of tools, the argument - is often treated to mean STDIN. Currently, Thor is unable to accept - as an argument value, since it defines a flag as any entry beginning with -. This seems overly restrictive. I would argue that a flag is defined as follows:

A flag is any entry that begins with 1 or 2 dashes, followed by at least one character

In regex terms, this is /-{1,2}\S+/.

TODO

  • update/create tests
  • update docs, if necessary (edit: does not appear necessary to update any docs)

@rafaelfranca

Copy link
Copy Markdown
Member

This makes sense. Can you write the tests?

test for array parsing of '-'
Comment on lines +43 to +47
it "accepts - as an array argument" do
create :array => nil
expect(parse("-")["array"]).to eq(%w(-))
expect(parse("-", "title", "-")["array"]).to eq(%w(- title -))
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I ran this test against master and, as expected, it fails. I don't think there's any other cases we need to consider for this feature.

I also went through the docs and feel like we don't need to update/change anything: IMO this change is to allow functionality that should already (implicitly) be present, not a new feature.

@timothysmith0609 timothysmith0609 marked this pull request as ready for review November 18, 2019 14:07
@timothysmith0609 timothysmith0609 changed the title 🌈 -WIP- More accurate Arguments#current_is_value? check 🌈 More accurate Arguments#current_is_value? check Nov 18, 2019
@rafaelfranca rafaelfranca merged commit 630bbee into rails:master Nov 22, 2019
@timothysmith0609 timothysmith0609 deleted the looser_flag_detection branch December 3, 2019 19:47
@timothysmith0609 timothysmith0609 mentioned this pull request Dec 16, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants