googleapis / python-bigquery-sqlalchemy

SQLAlchemy dialect for BigQuery
MIT License
426 stars 127 forks source link

feat: adds user agent parameters to two functions #1100

Closed chalmerlowe closed 1 month ago

chalmerlowe commented 1 month ago

This adds:

Fixes #1082 🦕

conventional-commit-lint-gcf[bot] commented 1 month ago

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot https://conventionalcommits.org/

Linchin commented 1 month ago

LGTM. Just some thoughts: should we add a system test to verify that user agent is indeed successfully set up?

chalmerlowe commented 1 month ago

LGTM. Just some thoughts: should we add a system test to verify that user agent is indeed successfully set up?

I am not inclined to put the time/resources to add a system test to cover this minor update.

Right now, when the to_user_agent() method is called...the customer-supplied user agent string has a variety of elements appended to it. These elements are dynamic and change depending on the versions of certain components that are installed.

Are you picturing an end-to-end system test that accounts for each element of this string? If so, then that type of test feels like it may be brittle and would be dependent on exactly what sub-components are present in this function and might involve complex checks like regex. There might be ways to simplify the checks.

If you have alternate ideas on how to do this simply, easily, and in a way that is robust to change within the code, you are welcome to suggest something.

if self.grpc_version is not None:
    ua += "grpc/{grpc_version} "

if self.rest_version is not None:
    ua += "rest/{rest_version} "

    ua += "gax/{api_core_version} "

if self.gapic_version is not None:
    ua += "gapic/{gapic_version} "

if self.client_library_version is not None:
    ua += "gccl/{client_library_version} "
Linchin commented 1 month ago

I will add a system test similar to the rest of tests in test_helpers.py. Sorry that I'm unable to suggest code change to locations unedited by this PR, apparently it's a feature request with github that hasn't been fulfilled. Feel free to modify as needed.