Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add with_name() to ScalarQueryParameterType #644

Merged
merged 2 commits into from May 5, 2021
Merged

feat: add with_name() to ScalarQueryParameterType #644

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2074bd5
feat: add with_name() to ScalarQueryParameterType
plamut May 3, 2021
6f57bc8
Clarify unsetting a name, add extra test
plamut May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions google/cloud/bigquery/query.py
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@

from collections import OrderedDict
import copy
from typing import Optional

from google.cloud.bigquery.table import _parse_schema_resource
from google.cloud.bigquery._helpers import _rows_from_json
Expand Down Expand Up @@ -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]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make name optional?

Copy link
Contributor Author

@plamut plamut May 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Return a copy of the instance with ``name`` set to ``new_name``.

Args:
name (Optional[str]): The new name of the query parameter type.

Returns:
google.cloud.bigquery.query.ScalarQueryParameterType:
A new instance with updated name.
"""
return type(self)(self._type, name=new_name, description=self.description)

def __repr__(self):
name = f", name={self.name!r}" if self.name is not None else ""
description = (
Expand Down
11 changes: 11 additions & 0 deletions tests/unit/test_query.py
View file
Open in desktop
Expand Up @@ -98,6 +98,17 @@ def test_repr_all_optional_attrs(self):
"ScalarQueryParameterType('BYTES', name='foo', description='this is foo')",
)

def test_with_name_returns_copy_w_changed_name(self):
param_type = self._make_one("BOOLEAN", name=None, description="Some checkbox.")
modified_type = param_type.with_name("allow_emails")

self.assertIsNot(modified_type, param_type) # Result is a copy.
self.assertEqual(modified_type.name, "allow_emails")

# The rest of the The rest of the fields should have been preserved.
self.assertEqual(modified_type._type, param_type._type)
self.assertEqual(modified_type.description, param_type.description)


class Test_ArrayQueryParameterType(unittest.TestCase):
@staticmethod
Expand Down