Skip to content
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

fix: adding property setter for table constraints, #1990 #2092

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

lkhagvadorj-amp
Copy link

@lkhagvadorj-amp lkhagvadorj-amp commented Dec 20, 2024

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)

Fixes #1990 🦕


Overview

This pull request introduces enhancements to the google/cloud/bigquery/table.py file by adding setter methods and conversion functions. Additionally, a new unit test is added to ensure the functionality of the new setter method.

Enhancements to google/cloud/bigquery/table.py:

  • Added a setter method for table_constraints to allow setting primary key and foreign key information.
  • Added to_api_repr method to the ForeignKey class to return a dictionary representation of the object.
  • Added to_api_repr method to the TableConstraints class to return a dictionary representation of the object.

Unit tests:

  • Added a test for the table_constraints property setter to ensure it correctly updates the table properties.

Copy link

google-cla bot commented Dec 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Dec 20, 2024
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Dec 20, 2024
@Linchin Linchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 20, 2024
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 20, 2024
@Linchin
Copy link
Contributor

Linchin commented Dec 20, 2024

Thank you @lkhagvadorj-amp for making the contribution! The PR looks great, but could you do the following things so we can merge it:

  1. sign the CLA because it's part of the legal requirements
  2. add some unit tests: unit tests for both to_api_repr() methods, and more test cases for the setter, for 100% test coverage
  3. add a system test for this property

Thanks again and happy holidays!

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 22, 2024
@lkhagvadorj-amp
Copy link
Author

@Linchin
Hi

Happy to contribute and thank you for the initial review!

I’ve pushed the testing changes—would be grateful if you could review them again.

Happy holidays!

@Linchin Linchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 30, 2024
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 30, 2024
@Linchin Linchin assigned Linchin and unassigned chalmerlowe Jan 2, 2025
api_repr = value
if isinstance(value, TableConstraints):
api_repr = value.to_api_repr()
elif value is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to add a unit test for this branch.

@Linchin
Copy link
Contributor

Linchin commented Jan 2, 2025

Thank you @lkhagvadorj-amp, I ran the test again, but it seems we need to fix several tests. I'm sorry that I have to trigger the presubmit tests manually, but you can run the system test locally by pytest tests/system/test_client.py, the lint test by nox -s lint, and mypy_samples test by nox -s mypy_samples. (You can install nox by pip install nox.)

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. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

table_constraints has no setter?
4 participants