-
Notifications
You must be signed in to change notification settings - Fork 6.5k
re-write authoring guide, add links to other documentation #2728
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
re-write authoring guide, add links to other documentation #2728
Conversation
CC @texasmichelle as an FYI |
AUTHORING_GUIDE.md
Outdated
upload_parser = subparsers.add_parser('upload', help=upload_blob.__doc__) | ||
upload_parser.add_argument('source_file_name') | ||
upload_parser.add_argument('destination_blob_name') | ||
Creating mocks for external resources is optional, and not recommended in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to add a bit here that says the exception generally is for long running operations (like AutoML). Best practice I've seen is to begin the operation, assert that the operation exists and is running, and then cancel the operation.
AUTHORING_GUIDE.md
Outdated
app.run(host='127.0.0.1', port=8080, debug=True) | ||
``` | ||
## Writing tests | ||
## Using nox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AUTHORING_GUIDE.md
Outdated
for a list of all environment variables used by all tests. Not every test | ||
needs all of these variables. | ||
|
||
## Google Cloud Storage Resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section might need an update, but I'm curious to hear other opinions. I feel like each sample may require resources more than just GCS - and they tend to specify the variables you need to fill in. I'm not sure that the ./resources section is kept up to date. I could be wrong though - so let's hear what the rest of the owners think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 this needs to be generalized to infrastructure resources as a whole.
|
||
Conceptually related samples under a service or API should be grouped into | ||
a subfolder. For example, App Engine Standard samples are under the | ||
[appengine/standard](https://github.com/GoogleCloudPlatform/python-docs-samples/tree/master/appengine/standard) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we sort based on runtime version as well? for example, appengine/standard/python3.7
?
AUTHORING_GUIDE.md
Outdated
|
||
Again, descriptive names prevent you from having to exhaustively describe | ||
every parameter. | ||
## Arrange, Act, Assert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a rule of thumb for the snippets themselves. Most snippets should use the follow pattern:
- Arrange - construct the API request and build the different components needed to configure the call
- Act - send the request to the server and wait for a response
- Assert - verify the response is a success, and act upon it (usually printing some results to show the call was successful)
AUTHORING_GUIDE.md
Outdated
|
||
subparsers = parser.add_subparsers(dest='command') | ||
## External Resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, any external resources that are required before the test (e.g. a Cloud SQL instance) should be pass in via an environment variable. This should be limited to infrastructure - if there is some data that need to be on that instance, the test should create it as part of the test (and clean it up when complete).
AUTHORING_GUIDE.md
Outdated
for a list of all environment variables used by all tests. Not every test | ||
needs all of these variables. | ||
|
||
## Google Cloud Storage Resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 this needs to be generalized to infrastructure resources as a whole.
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly. In order to pass this check, please resolve this problem and then comment ℹ️ Googlers: Go here for more info. |
2c26111
to
d94bf89
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
e647b97
to
d94bf89
Compare
description=__doc__, | ||
formatter_class=argparse.RawDescriptionHelpFormatter) | ||
parser.add_argument('bucket_name', help='Your cloud storage bucket.') | ||
Sample code may be integrated into Google Cloud Documentation through the use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May not be true across the board, but seems like our tracking tools suggest if the samples aren't in docs that we should be deleting them?
Love to hear what other folks are suggesting.
Here's a simple docstring for a command-line application with straightforward | ||
usage: | ||
The use of [Black](https://pypi.org/project/black/) to standardize code | ||
formatting and simplify diffs is recommended, but optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to put that they should adjust the defaults of black to use black -l 79
so that it matches PEP8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we try to match what is being used on google-cloud-python/ python-{product} as to not be divergent. A few flake8 rules are ignored (https://github.com/googleapis/python-storage/blob/master/.flake8) and black is used with defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to consistency with the client repos
``` | ||
|
||
### Functions & classes | ||
### Functions and Classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add default args to this section as an additional option?
def list_blobs(bucket_name="YOUR_BUCKET_NAME"):
21ee08c
to
8072a27
Compare
8072a27
to
f93453c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we'll keep tracking further improvements in the doc provided. Thanks Doug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic, @dmahugh !
Note that because many sections were added or completely re-written, the diff view is not helpful. This PR adds topics covered in the Java guide (for consistency), adds links to Google Cloud documentation where appropriate, and links to specific examples in existing samples where appropriate. Some sections have been removed to avoid duplication and simplify maintenance - for example, instead of showing the license header to use, it now links to the instructions in the LICENSE file for how to insert a license header.
Feedback welcome!