feat: add with_name() to ScalarQueryParameterType #644
Conversation
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: bigquery
google/cloud/bigquery/query.py
Outdated
| @@ -119,6 +120,18 @@ def to_api_repr(self): | |||
| # attributes in the API representation when needed. Here we omit them. | |||
| return {"type": self._type} | |||
|
|
|||
| def with_name(self, new_name: Optional[str]): | |||
There was a problem hiding this comment.
We allow explicit None to be consistent with the constructor that also accepts None. Perhaps using the alternative form Union[str, None] would make it less confusing, as the argument itself is not optional (to allow clearing the name).
There was a problem hiding this comment.
You're right that the argument is required. But I still don't see a point in allowing None. Nameless types are readily available in SqlParameterScalarTypes. To expect someone to call with_name and, you know, not provide a name seems silly. :)
I don't think changing Optional[str] to Union[str, None] is an improvement.
I'm willing to defer.
There was a problem hiding this comment.
I tend to not assume about the users' use cases too much, as several times in the past we have gotten that wrong. :)
I don't feel strongly about either way (supporting or not supporting clearing the name), but since the maintenance burden is more or less the same for both options, supporting None seems fine here.
Fixes #642.
PR checklist: