-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Updated pagination for jmespath result_key
s.
#232
Conversation
Looking, but have you seen the travis failure? We need to change the unittest import to |
@@ -57,3 +59,35 @@ def remove_dot_segments(url): | |||
output.append(url[:next_slash]) | |||
url = url[next_slash:] | |||
return ''.join(output) | |||
|
|||
|
|||
def exp_set(source, expression, value, is_first=True): |
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.
Can we rename this to something more descriptive? It's hard to tell what this does based on the name. jmespath_expression_set
, set_value_from_expression
, expression_set_value
, etc.
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.
Will rename.
I think the approach looks good overall. In addition to adding the pagination configs, don't forget we'd also need updated unittests showing that result_keys work with jmespath expressions. Feel free to update the PR when you'd like me to take another look. |
Looks good! |
for vals in zip_longest(*iterators): | ||
for k, val in zip(key_names, vals): | ||
if val is not None: | ||
response[k].append(val) | ||
response.setdefault(k.expression, []) | ||
response[k.expression].append(val) |
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.
Sorry, this was a miss on my part, we should not be setting any key names to k.expression
, we should be using set_value_from_jmespath
. For example if you run aws rds describe-engine-default-parameters --db-parameter-group-family mysql5.1
you'll see that the result is:
{
"EngineDefaults": {
"Parameters": []
},
"EngineDefaults.Parameters": [
{
"Description": "Controls whether user-defined functions that have only an xxx symbol for the main function can be loaded",
...
}
}
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.
Gah, sorry. :( I could get to this over the weekend, if that helps. The test I added is probably wrong too.
This is a WIP. I think the implementation is complete (unit tests are back to passing + additional tests), but we still need to update all the pagination configs. Before I add that to the PR, review of the approach/implementation would be appreciated.
@jamesls @danielgtaylor @garnaat