feat: add support for dataset.default_rounding_mode #1688
Conversation
| 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 bot added size: m 
Gaurang033 force-pushed the feature/default_rounding_mode branch from 590d65e to cf723d9 Compare 
Linchin requested review from Linchin and chalmerlowe and removed request for jainsahab Linchin added the kokoro:force-run 
yoshi-kokoro removed the kokoro:force-run 
google/cloud/bigquery/dataset.py Outdated
| """ | ||
| optional[str]: it could have one of the following possible value. | ||
| ROUNDING_MODE_UNSPECIFIED: Unspecified will default to using ROUND_HALF_AWAY_FROM_ZERO. | ||
| ROUND_HALF_AWAY_FROM_ZERO: ROUND_HALF_AWAY_FROM_ZERO rounds half values away from zero when applying precision | ||
| and scale upon writing of NUMERIC and BIGNUMERIC values. | ||
| For Scale: 0 1.1, 1.2, 1.3, 1.4 => 1 1.5, 1.6, 1.7, 1.8, 1.9 => 2 | ||
| ROUND_HALF_EVEN: ROUND_HALF_EVEN rounds half values to the nearest even value when applying precision and scale | ||
| upon writing of NUMERIC and BIGNUMERIC values. | ||
| For Scale: 0 1.1, 1.2, 1.3, 1.4 => 1 1.5 => 2 1.6, 1.7, 1.8, 1.9 => 2 2.5 => 2 | ||
| """ |
There was a problem hiding this comment.
Thank you for adding the docstring. I think its format is not passing the docs test, see testing logs for more detailed information. Here is an example for docstring format.
There was a problem hiding this comment.
done. i fixed it and run nox -s docs without any error.
| """ | ||
| return self._properties.get("defaultRoundingMode") | ||
| | ||
| @default_rounding_mode.setter |
There was a problem hiding this comment.
The test coverage is not passing because the setter function is not covered. Could you please also add unit test for this? Thank you!
There was a problem hiding this comment.
@Linchin should I modify this test_create_dataset_w_attrs ?
There was a problem hiding this comment.
I think it would be better to add several smaller tests (in parallel to test_create_dataset_with_default_rounding_mode() as you added), each for a case in our if conditions. These include:
valueisNonevalueis not a stringvaluenot inpossible_valuesvalueis inpossible_values
tests/system/test_client.py Outdated
| if kwargs.get("location"): | ||
| dataset.location = kwargs.get("location") | ||
| if kwargs.get("max_time_travel_hours"): | ||
| dataset.max_time_travel_hours = kwargs.get("max_time_travel_hours") |
There was a problem hiding this comment.
I see you have also opened #1683 that covers max_time_travel_hours. Let's make these changes over there, and confine this PR to only default_rounding_mode stuff.
There was a problem hiding this comment.
@Linchin done. cherry pick the commit to that pull request.
There was a problem hiding this comment.
Could you also delete these lines in this PR? Thank you!
| @Gaurang033 Thank you for your contribution! The PR looks good except for a few comments that need to be addressed. Please let us know if you need any help with them :) |
Gaurang033 force-pushed the feature/default_rounding_mode branch from dd54292 to 03becfe Compare google/cloud/bigquery/dataset.py Outdated
| Raises: | ||
| ValueError: for invalid value types. | ||
Gaurang033 force-pushed the feature/default_rounding_mode branch 3 times, most recently from 3c57278 to 5d43d87 Compare 
Gaurang033 force-pushed the feature/default_rounding_mode branch from 5d43d87 to b063e8a Compare Linchin added the kokoro:force-run yoshi-kokoro removed the kokoro:force-run Linchin added the kokoro:force-run yoshi-kokoro removed the kokoro:force-run | There's a mypy test error that's blocking this PR. Created #1705 to fix this. |
Linchin added the kokoro:force-run yoshi-kokoro removed the kokoro:force-run 
Co-authored-by: Lingqing Gan <lingqing.gan@gmail.com>
3 participants
Fixes #1687