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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

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

@vi3k6i5 vi3k6i5 requested a review from as a code owner May 12, 2021
@product-auto-label product-auto-label bot added the api: spanner label May 12, 2021
@google-cla google-cla bot added the cla: yes label May 12, 2021
@vi3k6i5 vi3k6i5 added kokoro:force-run and removed kokoro:force-run labels May 12, 2021
Copy link

@olavloite olavloite left a comment

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 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
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
larkee
larkee approved these changes May 17, 2021
Copy link
Contributor

@larkee larkee left a comment

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
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants