-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(bigquery): add description for routine entities #9785
feat(bigquery): add description for routine entities #9785
Conversation
/cc @shollyman |
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.
UPDATE: Nah, sorry, it does seem to work. I had to delete docs/generated
directory as well (and not just docs/_build
)
It seems that the description
property is present in the generated docs, but in not the attributes summary table (unlike other Routine
attributes).
Edit: Hmm, it might be a local cache issue, let me re-check. (nope, it is indeed missing from the attributes summary)
@@ -239,6 +240,17 @@ def body(self): | |||
def body(self, value): | |||
self._properties[self._PROPERTY_TO_API_FIELD["body"]] = value | |||
|
|||
@property | |||
def description(self): | |||
"""Union[str, None]: Description of the routine (defaults to |
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.
(nit)
Mypy docs recommend using the Optional
shorthand instead of a Union
with None
.
"""Union[str, None]: Description of the routine (defaults to | |
"""Optional[str]: Description of the routine (defaults to |
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.
ohk, should i change same in the other files which has description property like in dataset.py and model.py .
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.
Strictly speaking it is not in scope of this PR, but if it's not too much work, it would be a welcome cleanup IMO. 👍
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.
LGTM.
There are still usages of Union[foo, None]
, but with other attributes, thus let's leave that out for another time.
Fixes #9782