-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: Harmonize imports of sqlalchemy module, use sa
where applicable
#10
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
- Coverage 82.23% 82.12% -0.11%
==========================================
Files 7 7
Lines 681 677 -4
==========================================
- Hits 560 556 -4
Misses 121 121
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
import sqlalchemy | ||
import sqlalchemy as sa | ||
from crate.client.sqlalchemy.types import ObjectType, ObjectTypeImpl, _ObjectArray |
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.
That we need to import both ObjectType
and ObjectTypeImpl
here, and use them correspondingly, clearly indicates something is not optimal, and should be addressed on behalf of improvements to the type definitions of the CrateDB SQLAlchemy dialect in crate-python.
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.
👍
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.
LGTM
It follows a convention to import SQLAlchemy like `import sqlalchemy as sa`. In this spirit, all references, even simple ones like symbols to SQLAlchemy base types like `TEXT`, or `BIGINT`, will be referenced by `sa.TEXT`, `sa.BIGINT`, etc., so it is easy to tell them apart when harmonizing type definitions coming from SA's built-in dialects vs. type definitions coming from 3rd-party dialects.
74296f7
to
ab563dc
Compare
- Enforce `import typing as t` alias - Ban imports such as `from typing import Any`
Interesting comment by @edgarrmondragon at MeltanoLabs/target-postgres#255 (comment):
I didn't know about this detail of Ruff yet, thanks! 3e3c6ec adds the corresponding snippet. |
About
The patch follows a convention to import SQLAlchemy like
import sqlalchemy as sa
.In this spirit, all references, even simple ones like symbols to SQLAlchemy base types like
TEXT
, orBIGINT
, will be referenced bysa.TEXT
,sa.BIGINT
, etc., so it is easy to tell them apart when harmonizing type definitions coming from SA's built-in dialects vs. type definitions coming from 3rd-party dialects.References
sa
where applicable MeltanoLabs/target-postgres#255