-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
…-bigquery-connection into feat_python_quickstart Merge conflicts.
samples/snippets/quickstart.py
Outdated
if __name__ == "__main__": | ||
parser = argparse.ArgumentParser() | ||
parser.add_argument("--project_id", type=str) | ||
parser.add_argument("--location", default="US", type=str) | ||
args = parser.parse_args() | ||
main(project_id=args.project_id, location=args.location) |
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.
nit: Should this be included in the snippet? generally we favor not including CLIs in new samples
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.
Removed in latest commit!
samples/snippets/quickstart.py
Outdated
|
||
def main(project_id: str = "your-project-id", location: str = "US") -> None: | ||
# Constructs client for interacting with the service | ||
client = bq_connection.ConnectionServiceClient() |
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.
Can we push the client initialization down into list_connections
?
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.
Addressed in latest commit! I also removed the client fixture from conftest.py
though I'm guessing we'll need it with future samples!
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.
Looks good. Just one nit.
samples/snippets/quickstart.py
Outdated
if __name__ == "__main__": | ||
parser = argparse.ArgumentParser() | ||
parser.add_argument("--project_id", type=str) | ||
parser.add_argument("--location", default="US", type=str) | ||
args = parser.parse_args() | ||
main(project_id=args.project_id, location=args.location) |
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 was an old pattern. It's difficult to test the CLI arguments, so we omit them in our samples rubric. go/code-snippets-style#running-from-the-command-line
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.
Awesome, good to know - removed in latest commit!
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.
Thanks!
🤖 I have created a release \*beep\* \*boop\* --- ### [1.3.2](https://www.github.com/googleapis/python-bigquery-connection/compare/v1.3.1...v1.3.2) (2022-01-08) ### Documentation * add python quickstart sample ([#141](https://www.github.com/googleapis/python-bigquery-connection/issues/141)) ([8b85fb6](https://www.github.com/googleapis/python-bigquery-connection/commit/8b85fb6784ba9bf51123e9185f276391326dd54a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: