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

Allow for more than 256 occurrences of an argument. #409

Merged
merged 2 commits into from
Feb 2, 2016
Merged

Allow for more than 256 occurrences of an argument. #409

merged 2 commits into from
Feb 2, 2016

Conversation

cstorey
Copy link

@cstorey cstorey commented Feb 2, 2016

Hi,

I often use tools with xargs and the like, and it's entirely possible to end up with inhumanly long sets of parameters being passed to a command.

Without this fix, the library will panic with an arithmetic overflow in ArgMatcher#inc_occurrence_of when the 256th instance of the parameter is accounted for.

Thanks,

@yo-bot
Copy link

yo-bot commented Feb 2, 2016

Thanks for the pull request, and welcome! The team is excited to review your changes, and you should hear from @Vinatorul (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Vinatorul
Copy link
Contributor

Thank you for your contribution!
All looks good to me. Only one thing - we don't update changelog by ourselfs. We are using clog to generate it from commit messages.
Also, can you add yourself to Contributors.md list?
And the last one (really) - we are changing the licence from MIT to dual MIT/Apache 2. So, will you be so kind to accept it? Link to issue: #389

@cstorey
Copy link
Author

cstorey commented Feb 2, 2016

Hi, thanks for the quick review!
I've backed out the Changelog update; and added myself to the contributors file.
I'll confirm the license in the other ticket.

Thanks!

@Vinatorul
Copy link
Contributor

Thank you kindly! I will merge as soon as build finish.

@kbknapp
Copy link
Member

kbknapp commented Feb 2, 2016

Looks good to me too, and this is something I've been casually thinking about for a while so thanks! 👍 @homu r+

@homu
Copy link
Contributor

homu commented Feb 2, 2016

📌 Commit f86a183 has been approved by kbknapp

homu added a commit that referenced this pull request Feb 2, 2016
Allow for more than 256 occurrences of an argument.

Hi,

I often use tools with `xargs` and the like, and it's entirely possible to end up with inhumanly long sets of parameters being passed to a command.

Without this fix, the library will panic with an arithmetic overflow in `ArgMatcher#inc_occurrence_of` when the 256th instance of the parameter is accounted for.

Thanks,
@homu
Copy link
Contributor

homu commented Feb 2, 2016

⌛ Testing commit f86a183 with merge 700db3f...

homu added a commit that referenced this pull request Feb 2, 2016
Allow for more than 256 occurrences of an argument.

Hi,

I often use tools with `xargs` and the like, and it's entirely possible to end up with inhumanly long sets of parameters being passed to a command.

Without this fix, the library will panic with an arithmetic overflow in `ArgMatcher#inc_occurrence_of` when the 256th instance of the parameter is accounted for.

Thanks,
@homu
Copy link
Contributor

homu commented Feb 2, 2016

☀️ Test successful - status

@kbknapp
Copy link
Member

kbknapp commented Feb 2, 2016

@cstorey thanks again, and based on your work here I've also added support up to u64::max values per arg as well 😉 Once #408 merges I'll put out v2.0.3 on crates.io!

@cstorey
Copy link
Author

cstorey commented Feb 2, 2016

Thanks for your hard work!

homu added a commit that referenced this pull request Feb 3, 2016
Bug fixes, new setting, code clean up, and added support u64::max values per arg

This turned into a little more than just a quick bug fix 😜

Thanks to #409 I've piggy backed on that and added support for > 256 values as well.

There's also just a bunch of code deduping and super small macro cleanups.

After this merges I'll put out 2.0.3
homu added a commit that referenced this pull request Feb 3, 2016
Bug fixes, new setting, code clean up, and added support u64::max values per arg

This turned into a little more than just a quick bug fix 😜

Thanks to #409 I've piggy backed on that and added support for > 256 values as well.

There's also just a bunch of code deduping and super small macro cleanups.

After this merges I'll put out 2.0.3
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

5 participants