-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: handle error on some failing casts #5
Conversation
Thanks for this 👍 It'll need test coverage before being merged, given the challenge of supporting so many database combinations I'm keen to have full coverage of the data type processing |
Cheers, I don't know when I'll get the time too fire it up and check this over myself. I'd want the existing regression tests mentioned in #4 (comment) extended for every new data type that is handled. I'd be interested to know if you managed to run them. https://github.com/timabell/schema-explorer/blob/master/test.sh is the kick-off point for the full regression suite. |
On windows with no access to linux or WSL at the moment so those scripts
are of little use I'm afraid.
Den tors 31 okt. 2019 16:16Tim Abell <[email protected]> skrev:
… Cheers, I don't know when I'll get the time too fire it up and check this
over myself.
I'd want the existing regression tests mentioned in #4 (comment)
<#4 (comment)>
extended for every new data type that is handled. I'd be interested to know
if you managed to run them.
https://github.com/timabell/schema-explorer/blob/master/test.sh is the
kick-off point for the full regression suite.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5?email_source=notifications&email_token=AAAXS2QDE5HKBSSWTUL4EPLQRLZFHA5CNFSM4JHIFK7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECYE3ZI#issuecomment-548425189>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAXS2WDLDTGBCOYFO3LBRLQRLZFHANCNFSM4JHIFK7A>
.
|
Not all combinations tested yet.
Have now tested on sqlite and mssql at least. By looking at your sh scripts I created a powershell script and managed run tests for sqlite and mssql. Still haven't tested mysql or postgres. |
rebased on v0.67 and added a fix for #9 |
Now tested successfully on all drivers except mysql. That is probably related to issue #10 |
Using PR #14 this now passes the sse_test.go tests on sqlite, mssql, pg and postgres |
0582a2e
to
ea0626e
Compare
c97c00a
to
dd0b203
Compare
closing as this would need tidying up before I could merge it, and honestly my brain isn't entirely grokking what I'm looking at in the patch at the moment |
Closes #4