-
Notifications
You must be signed in to change notification settings - Fork 312
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
feature: raise error for unknown properties in job config #446
Conversation
google/cloud/bigquery/job/base.py
Outdated
def __setattr__(self, name, value): | ||
"""Override to be able to warn if an uknown property is being set""" | ||
if not name.startswith("_") and name not in type(self).__dict__: | ||
warnings.warn("Property {} is unknown for {}.".format(name, type(self))) |
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.
On second thought, I think we can go a step further and throw AttributeError
, just like object()
instances do.
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.
OK. Done.
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.
Thanks!
|
It appears you've uncovered a bug in the client. I don't see a setter for labels!
|
Nevermind, there is a setter:
Perhaps this logic needs to be adjusted in order to account for properties defined in the superclass? |
google/cloud/bigquery/job/base.py
Outdated
@@ -659,6 +659,12 @@ def __init__(self, job_type, **kwargs): | |||
for prop, val in kwargs.items(): | |||
setattr(self, prop, val) | |||
|
|||
def __setattr__(self, name, value): | |||
"""Override to be able to raise error if an unknown property is being set""" | |||
if not name.startswith("_") and name not in type(self).__dict__: |
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.
Let's try using hasattr to see if that accounts for superclass properties.
if not name.startswith("_") and name not in type(self).__dict__: | |
if not name.startswith("_") and not hasattr(self, name): |
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.
Ok, but we have to use type(self), because if not the properties are looked up on the instance.
One of our system tests actually had a mistake, which this change caught. There is no "destination" property on LoadJobConfig https://googleapis.dev/python/bigquery/latest/generated/google.cloud.bigquery.job.LoadJobConfig.html |
Re: labels error - as discussed offline, it's actually a bug in the base |
As far as I can tell, this has never been a valid config value here. But prior to the merging of [this PR][1] in v2.7.0, invalid attributes were silently ignored. [1]: googleapis/python-bigquery#446
As far as I can tell, this has never been a valid config value here. But prior to the merging of [this PR][1] in v2.7.0, invalid attributes were silently ignored. [1]: googleapis/python-bigquery#446
As far as I can tell, this has never been a valid config value here. But prior to the merging of [this PR][1] in v2.7.0, invalid attributes were silently ignored. [1]: googleapis/python-bigquery#446
As far as I can tell, this has never been a valid config value here. But prior to the merging of [this PR][1] in v2.7.0, invalid attributes were silently ignored. [1]: googleapis/python-bigquery#446
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:
Simple feature to raise warning if unknown property is set on job_config instances.
Fixes #303 🦕