Skip to content
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

MSSQL removes support column width/length on data type int #2738

Merged
merged 7 commits into from Mar 5, 2019

Conversation

kamote
Copy link
Contributor

@kamote kamote commented Jul 30, 2018

  • fixes error Cannot specify a column width on data type int

- fixes error `Cannot specify a column width on data type int`
- ignore length even if pass as agrument
elhigu
elhigu previously requested changes Aug 4, 2018
Copy link
Member

@elhigu elhigu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs integration test.

@kamote
Copy link
Contributor Author

kamote commented Aug 5, 2018

@elhigu Most of current integration test are now using .integer() w/o length
https://github.com/tgriesser/knex/blob/master/test/integration/schema/index.js#L546-L547

@elhigu
Copy link
Member

elhigu commented Aug 18, 2018

So if current integration tests are creating integers with specific width how come current integration tests are not throwing that error?

@kamote
Copy link
Contributor Author

kamote commented Aug 20, 2018

@elhigu upon looking, there is no MSSQL integration test that uses specific width which explains integration tests are not throwing.
In our case we found this kind of error since we are deploying same codebase with diff DB clients

@elhigu
Copy link
Member

elhigu commented Aug 20, 2018

@kamote thanks for looking that up. In that case such test should be added (regression test to make sure the same thing will not start failing again).

@kibertoad
Copy link
Collaborator

@kamote Could you give me write access on your repo so that I could add test?

@elhigu
Copy link
Member

elhigu commented Mar 4, 2019

@kibertoad did you try just to pull it and push changes? I have usually been able to do that... if it doesnt work, you can create your own PR also and link it here 👍

@kibertoad kibertoad dismissed elhigu’s stale review March 5, 2019 00:01

integration tests added

@kibertoad kibertoad merged commit c2bf7a3 into knex:master Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants