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 docstrings to resource actions and sub-resources #239

Merged
merged 5 commits into from
Oct 13, 2015

Conversation

kyleknap
Copy link
Member

@kyleknap kyleknap commented Sep 3, 2015

This adds lazy loaded docstrings to resource actions and sub-resources. Note that I still have yet to add docstrings identifiers, attributes, collections, and resource waiters. I just figured that splitting the work up a bit would be easier to review. There are a lot of moving parts so I figured to break it up to make it easier to follow and track feedback updates. Here are some examples of what you can do now:

import boto3

ec2 = boto3.resource('ec2')
instance = ec2.Instance('Id')

# Sub-resource
help(ec2.Instance)
# Load action
help(instance.load)
# Action         
help(instance.start)

Note that the tests won't pass till this PR gets merged: boto/botocore#642

cc @jamesls @mtdowling @rayluo

@rayluo rayluo added the enhancement This issue requests an improvement to a current feature. label Sep 11, 2015
@kyleknap kyleknap force-pushed the docstrings branch 2 times, most recently from 333f797 to ef5f96d Compare September 14, 2015 23:55
@kyleknap
Copy link
Member Author

The most recent updates add docstrings to identifiers, attributes, and references. Note that tests still won't pass until all of the botocore docstring PRs are merged.

@kyleknap kyleknap added documentation This is a problem with documentation. blocked labels Sep 14, 2015
@kyleknap kyleknap added pr/needs-review This PR needs a review from a member of the team. and removed blocked enhancement This issue requests an improvement to a current feature. labels Sep 18, 2015
@kyleknap
Copy link
Member Author

Cool. Since all of the botocore docstrings have been merged, this should be good to get reviewed. No longer relying on any outstanding PRs.

self._session = session
# I know that this is an internal attribute, but the botocore session
# is needed to load the paginator and waiter models.
self._botocore_session = session._session
Copy link
Member

Choose a reason for hiding this comment

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

I've noticed there's been a few scenarios where we need something from botocore's session. Should we start exposing some of the botocore session methods onto a boto3 session?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could look into this. I checked on where I use the botocore session. I use it for grabbing paginator models, waiter models, and the loader. Those are pretty low level as well so I am not sure if we want to expose those quite yet. I am not sure adding these to the boto3 session is in the scope of the this pr. Thoughts?

@jamesls
Copy link
Member

jamesls commented Sep 29, 2015

Looks good, just had a few comments.

@jamesls jamesls added incorporating-feedback and removed pr/needs-review This PR needs a review from a member of the team. labels Oct 12, 2015
@kyleknap
Copy link
Member Author

@jamesls I updated based on feedback. I did have a comment on exposing more methods on the boto3 session. I feel we should talk more about if the methods that we would need to expose to prevent accessing the _session attribute of a boto3 session is worth adding or if it is something we would want to expose. A fair amount of the methods that I do use on the botocore session are in the grey area in terms of what is deemed internal and what is external.

@kyleknap kyleknap added pr/needs-review This PR needs a review from a member of the team. and removed incorporating-feedback labels Oct 13, 2015
@jamesls
Copy link
Member

jamesls commented Oct 13, 2015

:shipit: Looks good. I agree we should probably talk more about what methods to expose in boto3's session, so we can do that independently of this PR.

@jamesls jamesls removed the pr/needs-review This PR needs a review from a member of the team. label Oct 13, 2015
@jamesls jamesls mentioned this pull request Oct 13, 2015
@jamesls jamesls added pr/ready-to-merge This PR is ready to be merged. and removed pr/ready-to-merge This PR is ready to be merged. labels Oct 13, 2015
kyleknap added a commit that referenced this pull request Oct 13, 2015
Add docstrings to resource actions and sub-resources
@kyleknap kyleknap merged commit 1a9134b into boto:develop Oct 13, 2015
@kyleknap kyleknap deleted the docstrings branch October 13, 2015 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. pr/ready-to-merge This PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants