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

Docstrings waiters #267

Merged
merged 12 commits into from
Nov 10, 2015
Merged

Docstrings waiters #267

merged 12 commits into from
Nov 10, 2015

Conversation

kyleknap
Copy link
Member

This is the final pull request in the series of pull requests that I have been making to add dynamically generated docstrings to clients and resources. I would recommend looking at this pull request last because it includes two previous pull requests, and thus makes it quite large.

In addition, I did a fair amount of refactoring of the resource factory code. With the waiter docstrings, I needed access to a waiter model and that really just added on to the really large signatures used throughout the factories. So I decided to shorten the signatures and make the arguments much more explicit to improve readability by reducing ambiguity. I shortened the signatures by introducing the concept of a ServiceContext where it is just a namedtuple of important information related to the service and adding many of the arguments to this new class. I did this because there was this pattern of where there were some objects that were needed for few spots in the code (and only for reading purposes), but due to the nature of how resources dangle off all other resources it needed to be passed to a dangling class so that it can be passed to other classes that may be generated from it. Thus this allows us to update the code such that if the factory needs any more information, we can add it to this context, instead of having to update the signatures and all of the spots the method is called.

cc @jamesls @mtdowling @rayluo @JordonPhillips

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

Test pass, but the rebasing job was messy here. I would prefer to wait until the collections docstrings are merged. So I can clean this up a bit more.

@kyleknap kyleknap added pr/needs-review This PR needs a review from a member of the team. blocked labels Oct 14, 2015
Cleaned up the signatures of the factory methods by combining read-only models
into a ServiceContext object. Also renamed args in the signature to have a
more explicit naming convention.
@kyleknap
Copy link
Member Author

Alright the commit history is much better now. No longer blocked for this one.

@jamesls
Copy link
Member

jamesls commented Nov 10, 2015

:shipit:

kyleknap added a commit that referenced this pull request Nov 10, 2015
@kyleknap kyleknap merged commit fdf0bd0 into boto:develop Nov 10, 2015
@kyleknap kyleknap deleted the docstrings-waiters branch November 10, 2015 19:06
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/needs-review This PR needs a review from a member of the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants