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

Follow SQLAlchemy dialect attribute convention #251

Closed
wants to merge 2 commits into from
Closed

Follow SQLAlchemy dialect attribute convention #251

wants to merge 2 commits into from

Conversation

jdimatteo
Copy link

@jdimatteo jdimatteo commented Aug 16, 2021 •
edited

Fixes #250

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. label Aug 16, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 16, 2021
@jdimatteo jdimatteo force-pushed the feature/follow-dialect-attribute-convention branch from aacb492 to 0631ccc Compare August 16, 2021 22:15
jdimatteo
jdimatteo commented Aug 16, 2021
View reviewed changes
sqlalchemy_bigquery/__init__.py
@@ -41,6 +41,9 @@
BIGNUMERIC,
)

# Define a "dialect" attribute to follow the common SqlAlchemy convention assumed by some libraries that leverage SqlAlchemy.
base.dialect = dialect = BigQueryDialect
Copy link
Author

Choose a reason for hiding this comment

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

If you'd like me to write a test, please let me know what file I should add the test to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I'd put this in tests/unit/test_sqlalchemy_bigquery.py.

jdimatteo
jdimatteo commented Aug 16, 2021
View reviewed changes
sqlalchemy_bigquery/__init__.py
@@ -41,6 +41,9 @@
BIGNUMERIC,
)

# Define a "dialect" attribute to follow the common SqlAlchemy convention assumed by some libraries that leverage SqlAlchemy.
Copy link
Author

Choose a reason for hiding this comment

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

Other SqlAlchemy dialects define a dialect attribute, including but not limited to:

See additional details on why this matters at #250.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I wasn't aware of this. Thanks!

This is a weird way to spell this. Why not define dialect in base and import it? 🤷

@jdimatteo jdimatteo marked this pull request as ready for review August 16, 2021 22:44
@jdimatteo jdimatteo requested a review from a team as a code owner August 16, 2021 22:44
This was referenced Aug 16, 2021
Merged
Closed
@jimfulton
Copy link
Contributor

I'll add the test...

@jimfulton
Copy link
Contributor

I'll add the test...

Well, no, I can't do that without creating a new PR.

jimfulton
jimfulton suggested changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@jimfulton jimfulton left a comment

Choose a reason for hiding this comment

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

Please add a test in tests/unit/test_sqlalchemy_bigquery.py.

It can be as simple as checking that the dialect is present as the dialect attribute in base and in the package. Should take 5 minutes to write. :)

@jimfulton
Copy link
Contributor

Superseded by #291

@jimfulton jimfulton closed this Aug 20, 2021
@jdimatteo
Copy link
Author

jdimatteo commented Aug 20, 2021 •
edited

Please add a test in tests/unit/test_sqlalchemy_bigquery.py.

It can be as simple as checking that the dialect is present as the dialect attribute in base and in the package. Should take 5 minutes to write. :)

Sorry I didn't do this sooner -- thanks for fixing this with #291!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimfulton jimfulton jimfulton requested changes

Assignees
No one assigned
Labels
api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include a dialect attribute to better follow common SqlAlchemy conventions relied upon by SqlAlchemy libraries
2 participants
@jdimatteo @jimfulton