-
Notifications
You must be signed in to change notification settings - Fork 139
Follow SQLAlchemy dialect attribute convention #251
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
Follow SQLAlchemy dialect attribute convention #251
Conversation
aacb492
to
0631ccc
Compare
@@ -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 |
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.
If you'd like me to write a test, please let me know what file I should add the test 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.
Thanks! I'd put this in tests/unit/test_sqlalchemy_bigquery.py
.
@@ -41,6 +41,9 @@ | |||
BIGNUMERIC, | |||
) | |||
|
|||
# Define a "dialect" attribute to follow the common SqlAlchemy convention assumed by some libraries that leverage SqlAlchemy. |
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.
Other SqlAlchemy dialects define a dialect
attribute, including but not limited to:
- base.dialect = dialect = snowdialect.dialect
- base.dialect = dialect = mysqldb.dialect
- base.dialect = dialect = psycopg2.dialect
See additional details on why this matters at #250.
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.
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? 🤷
I'll add the test... |
Well, no, I can't do that without creating a new PR. |
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.
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. :)
Superseded by #291 |
Sorry I didn't do this sooner -- thanks for fixing this with #291! |
Fixes #250