-
Notifications
You must be signed in to change notification settings - Fork 23
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 ? 43.99%
=======================================
Files ? 21
Lines ? 832
Branches ? 84
=======================================
Hits ? 366
Misses ? 461
Partials ? 5 β 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
π Linked issue
resolves #120
β Type of change
π Description
In this PR I've implemented an MSSQL connector utilizing the
tedious
library.π Checklist