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

Add doc string to PageIterator #2685

Merged
merged 3 commits into from Jun 6, 2022
Merged

Conversation

dlm6693
Copy link
Contributor

@dlm6693 dlm6693 commented May 25, 2022

Addressing #2396

While ideally we would add next functionality to PageIterator or simply update the name of the class to prevent confusion, both would cause breaking changes. As a result all we really can do is add a doc string with additional info. Perhaps using a warning here would be warranted, but decided against it.

Let me know if there's anything that should be added to the doc string.

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2022

Codecov Report

Merging #2685 (c7bd7e0) into develop (c7d4e40) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2685   +/-   ##
========================================
  Coverage    95.29%   95.29%           
========================================
  Files           60       60           
  Lines        12252    12252           
========================================
  Hits         11676    11676           
  Misses         576      576           
Impacted Files Coverage Δ
botocore/paginate.py 97.81% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7d4e40...c7bd7e0. Read the comment docs.

@dlm6693 dlm6693 changed the title Use next() on PageIterator Add doc string to PageIterator Jun 6, 2022
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Minor clarification and formatting tweaks. Otherwise, looks good!

botocore/paginate.py Outdated Show resolved Hide resolved
@@ -186,7 +186,8 @@ def get_paginator(self, operation_name):

class PageIterator:
"""An iterable object to pagiante API results.
Please note it is NOT a python iterator"""
Please note it is NOT a python iterator.
Use ``iter`` to wrap this as a generator"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure to include a period at the end of the last sentence? We'll also want to make sure the closing """ is on its own line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Should the opening quotes be on their own line too?

Copy link
Contributor

Choose a reason for hiding this comment

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

No we can keep it on the same line. The example in the first comment should have what's considered standard formatting for doc strings.

While we don't adhere strictly to PEP8 anymore, the conventions for docstrings are still widely used. Generally to the effect of:

class MyClass:
    """This is my class.
    
    Extra info beyond the class description.
    """

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it thanks! Pushing up now

@nateprewitt nateprewitt merged commit d99b1fd into boto:develop Jun 6, 2022
@dlm6693 dlm6693 deleted the paginator-iterator branch June 7, 2022 00:44
aws-sdk-python-automation added a commit that referenced this pull request Jun 7, 2022
* release-1.27.4:
  Bumping version to 1.27.4
  Update to latest endpoints
  Update to latest models
  Add doc string to `PageIterator` (#2685)
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.

None yet

3 participants