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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix erroneous float to integer conversion of decimal fields in MSSQL #1781

Merged
merged 2 commits into from Nov 30, 2016

Conversation

Projects
None yet
4 participants
@v12
Contributor

v12 commented Nov 15, 2016

Essentially, this is a fix for #1604 (also, relates to tediousjs/node-mssql#341).

When using Knex with MSSQL dialect, float values get truncated and, as a result, integer values get saved in decimal fields. This pull request fixes this invalid behaviour.

Feel free to comment or to suggest better solution 馃槈

@v12

This comment has been minimized.

Contributor

v12 commented Nov 15, 2016

Just in case, failing test doesn't relate to the changes made (it's something about timeouts )

@statyan

This comment has been minimized.

Contributor

statyan commented Nov 22, 2016

:) You did the same thing as I did for #1707. So this is a duplicate pull request. But thanks anyway for participating. I'm curious why my PR was not merged to the master...

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 24, 2016

Hi, I'm sorry about the delay. @statyan @v12 earlier pull request hasn't been merged just because I hoped that the guy who knows about the mssql implementation would review and comment on it. @Grimace1975 looks like you did the initial work for the mssql dialect, does these look fine?

Also there is no test case included for the fix.

Would be great if someone would take the ownership for the mssql dialect so that these pull requests would get more attention.

@smorey2

This comment has been minimized.

Collaborator

smorey2 commented Nov 24, 2016

@v12

This comment has been minimized.

Contributor

v12 commented Nov 24, 2016

@statyan ah, should've checked your PR before submitting this. Thanks for your contribution as well. Though, I noticed that you submitted multiple changes in a single PR - don't think this is the best way to do so because any mistake in one of the commits may block the whole PR from being merged. 鈿狅笍

@elhigu thanks for the reminder to add a test case, will push it asap 馃槈

@statyan

This comment has been minimized.

Contributor

statyan commented Nov 24, 2016

@v12 yep, it's my bad. Will never do the same thing again )
@Grimace1975 please review my PR #1707, there are several important fixes. Sorry for multiple fixes in one PR.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 24, 2016

@statyan I always do squash merge anyways for the new features and fixes, so multiple commits fixing stuff back and forth doesn't matter. After merge there will be just one clean commit 馃憤

@smorey2

This comment has been minimized.

Collaborator

smorey2 commented Nov 26, 2016

PR #1707 was merged into master. @v12 please merge these changes and review your code, your extra test should be added.

however, looks like there is still a test failing.

@v12

This comment has been minimized.

Contributor

v12 commented Nov 26, 2016

@Grimace1975 merged changes from master into the PR. Regarding failing Travis build, it fails only on Node 0.12 and relates to Babel and babel-plugin-lodash, which has nothing to do with this PR.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 27, 2016

CI has been really flaky lately, we (probably me) need to take some time and figure out how to get it stable again... that node 0.12 thing is quite strange.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 29, 2016

@Grimace1975 is this now good to be merged?

@v12

This comment has been minimized.

Contributor

v12 commented Nov 30, 2016

FYI, commit history has just been tidied up a bit

@smorey2 smorey2 merged commit 6789ffe into tgriesser:master Nov 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@v12 v12 deleted the v12:fix-mssql-decimal branch Nov 30, 2016

elhigu added a commit to elhigu/knex that referenced this pull request Feb 15, 2017

Fix erroneous float to integer conversion of decimal fields in MSSQL (t鈥
鈥riesser#1781)

* Fix erroneous float to integer conversion of decimal fields in MSSQL

* Add test case for decimal number handling in MSSQL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment