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.

Sign up for GitHub

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

Jump to bottom

Evade ValueError exception on ShorthandParserError #1543

Closed
wants to merge 1 commit into from
Closed

Evade ValueError exception on ShorthandParserError #1543

wants to merge 1 commit into from

Conversation

uchida
Copy link
Contributor

@uchida uchida commented Oct 4, 2015

When giving shorthand options like foo=bar',\nbar=bar", aws-cli got ValueError with message substring not found raised by string.rindex instead of ShorthandParserError message.

This pull request includes test code to reproduce, and evades raising ValueError and changes ShorthandParserError message format a little bit.

@rayluo rayluo added the pr:needs-review This PR needs a review from a Member. label Oct 5, 2015
@rayluo
Copy link
Contributor

rayluo commented Oct 5, 2015

Thanks. We will review this.

@uchida
Copy link
Contributor Author

uchida commented Oct 6, 2015

The message format change in 0cc582d seems inappropriate.
I will fix message format. Would you wait a review it?
If someone has started to review, sorry to bother.

@uchida uchida force-pushed the evade-exception-on-ShorthandParserError branch from 0cc582d to 4ffc347 Compare October 11, 2015 04:34
@uchida
Copy link
Contributor Author

uchida commented Oct 11, 2015

I have fixed message format and squashed.
Would you review it?

@uchida uchida force-pushed the evade-exception-on-ShorthandParserError branch from 4ffc347 to d1d27a1 Compare October 13, 2015 07:15
@jamesls
Copy link
Member

jamesls commented Oct 13, 2015

Taking a look...

@jamesls jamesls self-assigned this Oct 13, 2015
@jamesls
Copy link
Member

jamesls commented Oct 13, 2015

Merged, thanks for the pull request.

@jamesls jamesls closed this Oct 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jamesls jamesls

Labels
pr:needs-review This PR needs a review from a Member.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@uchida @rayluo @jamesls