Skip to content

Conversation

tswast
Copy link
Contributor

@tswast tswast commented Oct 11, 2021

Follow-up to GoogleCloudPlatform/python-docs-samples#6889, which removed a BigQuery magics sample for using query parameters.

Note: jupyter_tutorial_test.py is a copy of what is in the samples/snippets
folder. Once the docs have been updated to point to this new version, we can
remove that copy and remove the Jupyter/IPython depedencencies from
samples/snippets/requirements.txt.

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Note: jupyter_tutorial_test.py is a copy of what is in the `samples/snippets`
folder. Once the docs have been updated to point to this new version, we can
remove that copy and remove the Jupyter/IPython depedencencies from
`samples/snippets/requirements.txt`.
@tswast tswast requested review from loferris and a team October 11, 2021 15:38
@tswast tswast requested review from a team as code owners October 11, 2021 15:38
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 11, 2021
@snippet-bot
Copy link

snippet-bot bot commented Oct 11, 2021

Here is the summary of changes.

You are about to add 4 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added api: bigquery Issues related to the googleapis/python-bigquery API. samples Issues that are directly related to samples. labels Oct 11, 2021
@@ -0,0 +1,147 @@
# Copyright 2018 Google Inc. All Rights Reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware that this uses the natality dataset, which I think is on our list to migrate away from, but this is just to move the sample into a different directory from here: https://github.com/googleapis/python-bigquery/blob/main/samples/snippets/jupyter_tutorial_test.py

Samples are used here: https://cloud.google.com/bigquery/docs/visualize-jupyter

Possible we'll want to make this a regular notebook instead and put it here: https://github.com/GoogleCloudPlatform/bigquery-notebooks

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can go ahead with existing samples using natality as long as we circle back to it eventually.

I think that this would be a good candidate for a notebook when we have the embedded notebook pipeline set up. (Possibly Q4.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@loferris Perhaps I should remove this file from this PR? I was moving it to this directory mostly so I can cleanup the requirements.txt file in samples/snippets, but if we think we'll migrate this to a proper notebook soon-ish, then I can wait to clean that up until then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine as is!

Copy link
Contributor

@loferris loferris left a comment

Choose a reason for hiding this comment

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

Overall looks good, would love some context on writing a sample as a test file.

@@ -0,0 +1,147 @@
# Copyright 2018 Google Inc. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can go ahead with existing samples using natality as long as we circle back to it eventually.

I think that this would be a good candidate for a notebook when we have the embedded notebook pipeline set up. (Possibly Q4.)

# limitations under the License.
"""All of the samples used in the Jupyter notebooks tutorial.

Written as a test to save on boilerplate, since this sample has to similate
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good workaround for this kind of doc, but I would be interested in converting to a notebook as writing a sample as a test seems counterintuitive to maintain and fairly different from other samples of this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 on eventually converting to a notebook and integrating into a notebook->docs pipeline

@tswast tswast requested a review from loferris October 14, 2021 15:09
@tswast
Copy link
Contributor Author

tswast commented Oct 15, 2021

@loferris Could you mark this as Approved if there are no other changes I should make?

Copy link
Contributor

@loferris loferris left a comment

Choose a reason for hiding this comment

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

LGTM

@tswast tswast added the automerge Merge the pull request once unit tests and other checks pass. label Oct 29, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 61141ee into googleapis:main Oct 29, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 29, 2021
abdelmegahedgoogle pushed a commit to abdelmegahedgoogle/python-bigquery that referenced this pull request Apr 17, 2023
Follow-up to GoogleCloudPlatform/python-docs-samples#6889, which removed a BigQuery magics sample for using query parameters.

Note: jupyter_tutorial_test.py is a copy of what is in the `samples/snippets`
folder. Once the docs have been updated to point to this new version, we can
remove that copy and remove the Jupyter/IPython depedencencies from
`samples/snippets/requirements.txt`.

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:
- [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/python-bigquery/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [ ] Ensure the tests and linter pass
- [ ] Code coverage does not decrease (if any source code was changed)
- [ ] Appropriate docs were updated (if necessary)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants