Skip to content
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.

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

Fix a bug that flag value is processed as flag #241

Merged
merged 1 commit into from
Jun 29, 2019

Conversation

kou
Copy link
Contributor

@kou kou commented Jun 24, 2019

If flag value starts with "-", unknown option error is raised.

The current flag value check is "orig_arg == opt.value.to_s". There
are some objects such as Regexp and Time that input value and its #to_s
aren't same.

@leejarvis
Copy link
Owner

Thanks @kou -- please could you check the Travis failures and tweak your changes to make the build green? I'll take a closer look afterwards

@kou kou force-pushed the fix-unconsumed-argument branch 2 times, most recently from 5c4cc07 to 7e76b6d Compare June 25, 2019 15:01
@kou
Copy link
Contributor Author

kou commented Jun 25, 2019

Oh sorry. I've fixed the failure. We can't use Regexp#match? with old Ruby.

Copy link
Owner

@leejarvis leejarvis left a comment

Choose a reason for hiding this comment

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

Added a couple of comments but happy to merge after those. Thanks for fixing the build!

@@ -106,6 +106,13 @@ def unused_options

private

def consume_next_argument?(flag)
return false if /=/ === flag
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use flag.include? here instead? I think it's more obvious

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. Done.

@@ -49,6 +49,12 @@
assert_equal(-123.987, @result[:multiple])
end

it "parses negative float" do
Copy link
Owner

Choose a reason for hiding this comment

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

This test description isn't right, a c+p I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... Sorry.
I've fixed the description and move this test under "parses arg with leading -". Because they are related.

@kou kou force-pushed the fix-unconsumed-argument branch from 7e76b6d to 9ff7724 Compare June 28, 2019 21:09
@kou
Copy link
Contributor Author

kou commented Jun 28, 2019

I've pushed fixes by amending the original commit.
If you like to add new commits, sorry for this. I'll add new commits at the next time.

If flag value starts with "-", unknown option error is raised.

The current flag value check is "orig_arg == opt.value.to_s". There
are some objects such as Regexp and Time that input value and its #to_s
aren't same.
@kou kou force-pushed the fix-unconsumed-argument branch from 9ff7724 to c026ee9 Compare June 28, 2019 21:18
@leejarvis
Copy link
Owner

The force-push is fine. I'll get this merged. Thanks!

@leejarvis leejarvis merged commit 92dafc9 into leejarvis:master Jun 29, 2019
@kou kou deleted the fix-unconsumed-argument branch June 29, 2019 22:07
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.

None yet

2 participants