-
Notifications
You must be signed in to change notification settings - Fork 302
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
feat: refactor AccessEntry to use _properties pattern #1125
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good! I made a few suggestions to use get(...)
, _get_sub_prop
, and _set_sub_prop
.
I think we'll need some more unit tests that exercise getting the various properties when they aren't set. Make sure they get None
(or whatever the desired default is) instead of raising a KeyError
.
…ess-entry-refactor
Co-authored-by: Tim Swast <swast@google.com>
Co-authored-by: Tim Swast <swast@google.com>
Co-authored-by: Tim Swast <swast@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! A few small things
Co-authored-by: Tim Swast <swast@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! I think the behavior is right now. Just a nit re: what we're storing in _properties
. I'd like us to stay consistent with the other resource classes.
google/cloud/bigquery/dataset.py
Outdated
def dataset(self) -> Optional[DatasetReference]: | ||
"""API resource representation of a dataset reference.""" | ||
return typing.cast( | ||
Optional[DatasetReference], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually like to see _properties
store the REST API version and the property getter call from_api_repr
. Same for the other properties.
…ess-entry-refactor
…gquery into access-entry-refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good! I'll approve, but there are a few edge cases you'll want to handle before merging.
Co-authored-by: Tim Swast <swast@google.com>
Co-authored-by: Tim Swast <swast@google.com>
Co-authored-by: Tim Swast <swast@google.com>
…gquery into access-entry-refactor
* add view, dataset, routine properties * add properties and unit tests * lint * add properties and tests * remove unused imports * update test * add tests * add tests * add tests * add more tests * update return type * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * refactor to use get() and remove self._entity_id * delete unnecessary file * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * add types, remove unnecessary checks * fix types * types * add type casting * refactor AccessEntry repr * update to return DatasetReference * update to use RoutineRef and TableRef * add table test * update to use api_repr * lint * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * update tests * remove repeat type import Co-authored-by: Tim Swast <swast@google.com>
* add view, dataset, routine properties * add properties and unit tests * lint * add properties and tests * remove unused imports * update test * add tests * add tests * add tests * add more tests * update return type * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * refactor to use get() and remove self._entity_id * delete unnecessary file * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * add types, remove unnecessary checks * fix types * types * add type casting * refactor AccessEntry repr * update to return DatasetReference * update to use RoutineRef and TableRef * add table test * update to use api_repr * lint * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * Update google/cloud/bigquery/dataset.py Co-authored-by: Tim Swast <swast@google.com> * update tests * remove repeat type import Co-authored-by: Tim Swast <swast@google.com>
Fixes #1077 🦕