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

Only display --page-size when operation is paginated #956

Closed
wants to merge 2 commits into from

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Oct 17, 2014

This moves the --page-size CLI option to the pagination customization.
Before adding the argument, we first verify that not only can the
operation be paginated, but also that the pagination configuration
supports a limit key. This also means that the --page-size argument
is now documented on the man/html page for each operation that supports
pagination, making it more clear when this argument can be used.

The only other update was that I had to add a page size to the
S3 plugin as well.

cc @danielgtaylor @kyleknap

@jamesls jamesls force-pushed the page-size-in-pagination branch from d1e885b to 96f3fa1 Compare October 17, 2014 23:22
@kyleknap
Copy link
Contributor

The only comment I have is would it make sense to include --page-size in rm since it is included in all of the other transfer commands? Page size for an rm command would really only dictate how many objects are returned for each list objects call. But that is the same vote for the rest of the transfer commands like cp, mv, and sync etc. Also, note that if --page-size is not added as a valid argument it would be a change from existing behavior, since before --page-size was a global.

Otherwise, it looks good. 🚢 Thanks for running the integration tests.

@jamesls
Copy link
Member Author

jamesls commented Oct 20, 2014

Makes sense. I'll update this to include --page-size for the rm command.

This moves the --page-size CLI option to the pagination customization.
Before adding the argument, we first verify that not only can the
operation be paginated, but also that the pagination configuration
supports a limit key.  This also means that the --page-size argument
is now documented on the man/html page for each operation that supports
pagination, making it more clear when this argument can be used.

The only other update was that I had to add a page size to the
S3 plugin as well.
The rm command does make a ListObjects call in the recursive case,
so it makes sense to be consistent and support the --page-size argument
for aws s3 rm.
@jamesls jamesls force-pushed the page-size-in-pagination branch from 96f3fa1 to 15d8f4a Compare October 20, 2014 23:50
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 15d8f4a on jamesls:page-size-in-pagination into c2de08b on aws:develop.

@jamesls
Copy link
Member Author

jamesls commented Oct 21, 2014

@kyleknap Added --page-size to the s3 rm command. Also added an integ test for this feature as well.

@kyleknap
Copy link
Contributor

Thanks looks good. 🚢

jamesls added a commit that referenced this pull request Oct 31, 2014
@jamesls
Copy link
Member Author

jamesls commented Oct 31, 2014

Merged via accc815

@jamesls jamesls closed this Oct 31, 2014
kyleknap added a commit that referenced this pull request Nov 4, 2014
* release-1.5.5: (28 commits)
  Bumping version to 1.5.5
  [EMR] Update create-cluster synopsis
  [EMR] Update the EMRFS bootstrap action name to be 'Setup EMRFS'
  [EMR] Remove create-cluster --applications default value and update EMRFS bootstrap action name
  [EMR] Correct MapR AMI version in create-cluster examples
  Update docs with shared credentials changes
  Rename aws_security_token to aws_session_token
  Fix integration test
  Update changelog with fix for #956
  Add --page-size argument to the aws s3 rm command
  Only display --page-size when operation is paginated
  Updated CHANGELOG
  add frankfurt region
  Fix typo in error message
  Updated the travis yml for unittest2
  Refactor the clidriver event system
  Unit and integration test pass for json skeleton
  Fix arguments with no required setters
  Accepts input shape of None
  Add generate skeleton integ test
  ...
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.

3 participants