-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Docstrings waiters #267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8021128
to
d897176
Compare
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. |
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.
d897176
to
b4353ca
Compare
Alright the commit history is much better now. No longer blocked for this one. |
|
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.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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