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.

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

Enable use of page_size for clients #408

Merged
merged 2 commits into from
Dec 10, 2014
Merged

Conversation

kyleknap
Copy link
Contributor

@kyleknap kyleknap commented Dec 9, 2014

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

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

from tests import unittest
from botocore.paginate import Paginator as FuturePaginator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@jamesls
Copy link
Member

jamesls commented Dec 9, 2014

:shipit: 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
Copy link

Coverage Status

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

@danielgtaylor
Copy link
Member

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
Copy link
Member

jamesls commented Dec 9, 2014

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

@kyleknap
Copy link
Contributor Author

kyleknap commented Dec 9, 2014

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

@kyleknap
Copy link
Contributor Author

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

kyleknap added a commit that referenced this pull request Dec 10, 2014
Enable use of page_size for clients
@kyleknap kyleknap merged commit 7a2181f into boto:develop Dec 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants