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

Add more cases to comprehensive parser test. Mainly for -- . #266

Merged
merged 1 commit into from
Sep 30, 2015
Merged

Add more cases to comprehensive parser test. Mainly for -- . #266

merged 1 commit into from
Sep 30, 2015

Conversation

geophree
Copy link
Contributor

Another argument parsing library had a problem with -- after --, here I add extra data to test that and various other edge cases in your comprehensive parser test.

@nexdrew
Copy link
Member

nexdrew commented Sep 29, 2015

@geophree Thanks!

So you just want to expand the "short-circuit option parsing" test data to make sure we don't regress on handling -- after -- in the future?

Not sure this provides a whole lot of value, but I'll buy it for a dollar. LGTM 👍 Thanks!

@geophree
Copy link
Contributor Author

Yep, that's exactly it. Your test suite is pretty comprehensive, I figured I'd push it a little closer to "completely" comprehensive. As I mentioned, another command parser (who shall remain nameless) had a problem with this case. I submitted a PR to fix their implementation & test as well. Thanks for making such an awesome arg parsing library!

@nexdrew
Copy link
Member

nexdrew commented Sep 29, 2015

Ahh, gotcha. Nice work!

Most of the credit for yargs goes to @bcoe, @substack, and @chevex. Indeed, they have built an awesome library. I'm just along for the ride. 😉

Thanks, again, for contributing back!

@bcoe
Copy link
Member

bcoe commented Sep 30, 2015

@geophree thanks for the contribution and kind words \o/ :tada:

There's nothing better in OSS than having someone add test cases for you.

bcoe added a commit that referenced this pull request Sep 30, 2015
Add more cases to comprehensive parser test.  Mainly for -- .
@bcoe bcoe merged commit a79900a into yargs:master Sep 30, 2015
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

3 participants