Enable use of page_size for clients #408

kyleknap · 2014-12-09T21:55:02Z

So the reason I made this pull request is because, you cannot paginate using a page size with clients. This is what the error you get before my PR:

>>> import botocore.session
>>> session = botocore.session.get_session()
>>> s3 = session.create_client('s3')
>>> paginator = s3.get_paginator('list_objects')
>>> for page in paginator.paginate(Bucket='mybucketfoo', page_size=1):
...     print page
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/kyleknap/Documents/GitHub/botocore/botocore/paginate.py", line 67, in __iter__
    self._inject_starting_params(current_kwargs)
  File "/Users/kyleknap/Documents/GitHub/botocore/botocore/paginate.py", line 140, in _inject_starting_params
    page_size_name = self._operation.pagination['limit_key']
AttributeError: 'PageIterator' object has no attribute '_operation'

This also probably affects boto3 since it uses clients to paginate as well.

The fix was to retrieve the limit_key from the pagination config instead of relying on an Operation object for the limit_key, which the clients had no access to.

cc @jamesls @danielgtaylor

kyleknap · 2014-12-09T21:58:43Z

tests/unit/test_paginate.py

@@ -12,6 +12,7 @@
 # language governing permissions and limitations under the License.

 from tests import unittest
+from botocore.paginate import Paginator as FuturePaginator


Since the Paginator name is clobbered by the renaming of DeprecatedPaginator, I had to rename the paginator that we will eventually be using. I considered switching everything over to the paginator will be using in the future. However the CLI relies on the deprecated paginators, so I was hesitant to make the switch.

Might be nice to add this as a comment in the test so we don't forget why this was done.

jamesls · 2014-12-09T22:30:15Z

Looks good.

On a completely separate note, I'm finding it jarring that we use CamelCased and snake_cased params for pagination. I'm thinking for the client we should be consistent and use the CamelCased params for the pagination values as well. Thoughts? cc @danielgtaylor

coveralls · 2014-12-09T22:31:48Z

Coverage decreased (-1.04%) when pulling 86ce5fd on kyleknap:client-page-size into 459da7f on boto:develop.

danielgtaylor · 2014-12-09T22:46:06Z

LGTM 🚢-it.

CamelCased or snake_cased are both fine for me. It seems possible that some service in the future might not use CamelCasing for its args, and then we'll have the opposite problem we do today. If we want to go with CamelCasing for now then that's fine since (all?) services use CamelCased params.

jamesls · 2014-12-09T22:58:14Z

I don't think a service will ever use snake_casing.

kyleknap · 2014-12-09T23:02:56Z

+1 CamelCase since we are moving away from service and operation objects, which accept snake_case.

kyleknap · 2014-12-10T18:40:20Z

I added a todo comment to help us later on when we switched the test to use the non deprecated paginator. Merging.

Enable use of page_size for clients

Enable use of page_size for clients

86ce5fd

kyleknap reviewed Dec 9, 2014
View reviewed changes

Add a todo commnet for the test

b876203

kyleknap added a commit that referenced this pull request Dec 10, 2014

Merge pull request #408 from kyleknap/client-page-size

7a2181f

Enable use of page_size for clients

kyleknap merged commit 7a2181f into boto:develop Dec 10, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable use of page_size for clients #408

Enable use of page_size for clients #408

Enable use of page_size for clients #408

Enable use of page_size for clients #408

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment