-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: MSSQL connector #121
base: main
Are you sure you want to change the base?
Conversation
β¦er than rows changed to account for queries that don't return data
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
=======================================
Coverage ? 42.87%
=======================================
Files ? 23
Lines ? 1024
Branches ? 106
=======================================
Hits ? 439
Misses ? 581
Partials ? 4 β View full report in Codecov by Sentry. |
src/connectors/mssql/connector.ts
Outdated
} | ||
|
||
if (value instanceof Date) { | ||
return TYPES.DateTime; |
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 is also a DateTime2 type in tedious and in SQL
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.
@peterbud This code was taken directly from the Kysely library, but upon research I do see that DateTime2
is a more suitable suggestion for anything new going forward. The MSSQL documentation says that it aligns with the existing SQL standard and should be able to be used as a drop-in replacement.
Would you prefer if we use DateTime2 as the default here instead?
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.
I think that would be better. datetime has issues with fractional milliseconds.
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.
@peterbud I've pushed up a commit to swap this value to use DateTime2. Let me know if this works for you.
Thanks!
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
Hi @nick-w-nick, thanks for your work on this. sorry it might take me longer to be able to properly review and test this PR but I have not forgotten. |
β¦on-camelcaseable connector names
@pi0 I've gone ahead and updated the PR to use the new typings/export logic that had been merged in previously. All merge conflicts should be the resolved and the tests should now pass once more. If you need it to test locally, this is the Docker image I am using for MSSQL as it is the most compatible across platforms: These are the environment variables required (also shown in the MSSQL_HOST=localhost
MSSQL_PORT=1433
MSSQL_USERNAME=sa # default mssql username
MSSQL_PASSWORD=password
MSSQL_DB_NAME=test_db Let me know if you need any additional assistance! |
π Linked issue
resolves #120
β Type of change
π Description
In this PR I've implemented an MSSQL connector utilizing the
tedious
library.π Checklist