FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

re-write authoring guide, add links to other documentation by dmahugh · Pull Request #2728 · GoogleCloudPlatform/python-docs-samples · GitHub

re-write authoring guide, add links to other documentation - #2728

Merged
kurtisvg merged 3 commits into
GoogleCloudPlatform:masterfrom
dmahugh:style-guide-cleanup
Apr 3, 2020
Merged

re-write authoring guide, add links to other documentation#2728
kurtisvg merged 3 commits into
GoogleCloudPlatform:masterfrom
dmahugh:style-guide-cleanup

Conversation

dmahugh commented Jan 16, 2020

Copy link
Copy Markdown
Contributor

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!

dmahugh requested a review from a team as a code owner January 16, 2020 02:21
googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 16, 2020

dmahugh commented Jan 16, 2020

Copy link
Copy Markdown
Contributor Author

CC @texasmichelle as an FYI

Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This section may change a little bit if we integrate @kurtisvg 's propsed noxfile changes #2806

Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 this needs to be generalized to infrastructure resources as a whole.

Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we sort based on runtime version as well? for example, appengine/standard/python3.7?

Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is actually a rule of thumb for the snippets themselves. Most snippets should use the follow pattern:

  1. Arrange - construct the API request and build the different components needed to configure the call
  2. Act - send the request to the server and wait for a response
  3. Assert - verify the response is a success, and act upon it (usually printing some results to show the call was successful)

Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

Comment thread AUTHORING_GUIDE.md Outdated
Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 this needs to be generalized to infrastructure resources as a whole.

Copy link
Copy Markdown

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@googlebot I fixed it... If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Feb 14, 2020
dmahugh force-pushed the style-guide-cleanup branch from 2c26111 to d94bf89 Compare February 14, 2020 20:02

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 14, 2020
dmahugh force-pushed the style-guide-cleanup branch from e647b97 to d94bf89 Compare February 14, 2020 20:07
Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would it be helpful to put that they should adjust the defaults of black to use black -l 79 so that it matches PEP8?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 to consistency with the client repos

Comment thread AUTHORING_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we add default args to this section as an additional option?

def list_blobs(bucket_name="YOUR_BUCKET_NAME"):

kurtisvg force-pushed the style-guide-cleanup branch from 8072a27 to f93453c Compare April 2, 2020 21:36

kurtisvg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, we'll keep tracking further improvements in the doc provided. Thanks Doug!

leahecole left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is fantastic, @dmahugh !

kurtisvg merged commit b5c8ae1 into GoogleCloudPlatform:master Apr 3, 2020
kurtisvg deleted the style-guide-cleanup branch April 3, 2020 19:49
This file contains hidden or 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create sample format guidelines for Python-docs-samples

7 participants


Back | FazBrowse Home | New Git URL