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

Fix pagination with string params #689

Merged
merged 4 commits into from
Mar 5, 2014
Merged

Fix pagination with string params #689

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2443277
Fix pagination with string params
danielgtaylor Mar 4, 2014
0ee286e
More useful error and added tests
danielgtaylor Mar 4, 2014
bc02513
Add Route53 max items test case
danielgtaylor Mar 5, 2014
05044cc
Use public rather than private members
danielgtaylor Mar 5, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion awscli/customizations/paginate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,19 @@ def unify_paging_params(argument_table, operation, event_name, **kwargs):
STARTING_TOKEN_HELP,
operation,
parse_type='string')
# Try to get the pagination parameter type
type_ = 'integer'
if 'limit_key' in operation.pagination:
for param in operation.params:
if param.name == operation.pagination['limit_key']:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a safe assumption that there's always limit_key, based on the code below that does a if 'limit_key' in operation.pagination. It would be worth double checking that this is the case. Either way we should be consistent in this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is line 71 not the same as 107? I'm not sure what you mean here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, sorry I misread the code. Disregard my previous comment.

type_ = param.type
break

if type_ not in PageArgument.type_map:
raise TypeError('Unsupported pagination type {0}'.format(type_))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's include more context in the error message (the operation/parameter name would be helpful).


argument_table['max-items'] = PageArgument('max-items', MAX_ITEMS_HELP,
operation, parse_type='integer')
operation, parse_type=type_)


def check_should_enable_pagination(input_tokens, parsed_args, parsed_globals, **kwargs):
Expand Down
4 changes: 4 additions & 0 deletions tests/unit/customizations/test_paginate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ def setUp(self):
self.foo_param = mock.Mock()
self.foo_param.cli_name = 'foo'
self.foo_param.name = 'Foo'
self.foo_param.type = 'string'
self.bar_param = mock.Mock()
self.bar_param.cli_name = 'bar'
self.bar_param.type = 'string'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for:

  1. integer
  2. Unknown type (the raising TypeError is not tested).

self.bar_param.name = 'Bar'
self.params = [self.foo_param, self.bar_param]
self.operation.pagination = {
Expand Down Expand Up @@ -54,6 +56,8 @@ def test_customize_arg_table(self):
paginate.PageArgument)
self.assertIsInstance(argument_table['max-items'],
paginate.PageArgument)
# Max items should be the same type as bar, which may not be an int
self.assertEqual('string', argument_table['max-items']._parse_type)

def test_operation_with_no_paginate(self):
# Operations that don't paginate are left alone.
Expand Down