-
Notifications
You must be signed in to change notification settings - Fork 783
Coerce operation params according to model #147
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
Conversation
10c2d4c to
af191c2
Compare
awsshell/compat.py
Outdated
| else: | ||
| from HTMLParser import HTMLParser | ||
| text_type = unicode | ||
| long_type = long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would six help with some of these? Could help to notice that you have a text_type as well which six has. Furthermore, we really do not worry about longs in botocore so you can just get away with six.integer_types: https://github.com/boto/botocore/blob/develop/botocore/validate.py#L235-L237
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The six.integer_types is (int, long) in python 2 and just int in python 3. This makes it not possible to use the value as a constructor to coerce the parameter. That being said after doing some research it seems that calling int on a large enough number will end up returning a long anyways and this may not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it might be okay to map the types 'long' and 'int' to just use int in both python 2 and 3.
This is the behavior in python 2:
int('99999999999999999999') => 99999999999999999999L
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
|
Approach looks fine. I just had a few questions. Parameter translation based off model traversal has a fair amount of edge cases and there is a bunch of places in botocore where we do it so I would make sure you take a look at those. The places we do it notably is: serializers, response parsers, and documentation. |
|
It really is a shame that we can't use the CLI, but this is a fairly well known problem as Kyle mentioned. Looks good for the most part, just a few small comments. |
|
|
Looks good. I am not sure about a better name than coerce but 🚢 if you can't think of a better name either. |
This pull requests adds a basic framework for coercing parameters into the correct type. Input taken from prompts will always be strings and for some basic types it is easy enough to cast them implicitly rather than having to define explicitly what type it is in the wizard specification or have a custom interaction.