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

Jump to bottom

feat: add decimal validation for numeric precision and scale supported by Spanner #340

Merged
merged 13 commits into from May 18, 2021
Merged

feat: add decimal validation for numeric precision and scale supported by Spanner #340

merged 13 commits into from May 18, 2021

Conversation

vi3k6i5
Copy link
Contributor

@vi3k6i5 vi3k6i5 commented May 12, 2021 •
edited

feat: add decimal validation for numeric precision and scale supported by spanner
Fixes #339 🦕

@vi3k6i5 vi3k6i5 requested a review from a team as a code owner May 12, 2021 10:27
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label May 12, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 12, 2021
@vi3k6i5 vi3k6i5 added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 12, 2021
olavloite
olavloite approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

This seems good to my untrained Python eyes, with a small question on whether it is logical to call the validate function for all values, or whether it would be better to move the if-check for the type out of the validation function.
(You should in any case await approval from other reviewers before merging)

google/cloud/spanner_dbapi/parse_utils.py Outdated Show resolved Hide resolved
@vi3k6i5
Copy link
Contributor Author

vi3k6i5 commented May 14, 2021

This seems good to my untrained Python eyes, with a small question on whether it is logical to call the validate function for all values, or whether it would be better to move the if-check for the type out of the validation function.
(You should in any case await approval from other reviewers before merging)

moved the method after a check. and also moved it to base client as @larkee suggested.

@vi3k6i5 vi3k6i5 requested a review from larkee May 14, 2021 16:17
larkee
larkee reviewed May 17, 2021
View reviewed changes
google/cloud/spanner_v1/_helpers.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_helpers.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_helpers.py Outdated Show resolved Hide resolved
tests/unit/test__helpers.py Outdated Show resolved Hide resolved
@vi3k6i5 vi3k6i5 requested a review from larkee May 17, 2021 07:37
larkee
larkee approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

LGTM 👍 You could remove test_w_numeric if you wanted since test_w_numeric_precision_and_scale_valid should have more coverage.

@larkee larkee changed the title feat: add decimal validation for numeric precission and scale supported by spanner feat: add decimal validation for numeric precision and scale supported by Spanner May 17, 2021
@vi3k6i5 vi3k6i5 merged commit aa36c5e into googleapis:master May 18, 2021
This was referenced Jun 15, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olavloite olavloite olavloite approved these changes

@larkee larkee larkee approved these changes

@skuruppu skuruppu Awaiting requested review from skuruppu

@thiagotnunes thiagotnunes Awaiting requested review from thiagotnunes

@c24t c24t Awaiting requested review from c24t

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/python-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add validation in decimal to numeric conversion
3 participants
@vi3k6i5 @olavloite @larkee