Skip to content

fix(mssql): set correct scale for float#11962

Merged
sushantdhiman merged 2 commits intosequelize:masterfrom
JohnOnLee:issue/11959
Mar 16, 2020
Merged

fix(mssql): set correct scale for float#11962
sushantdhiman merged 2 commits intosequelize:masterfrom
JohnOnLee:issue/11959

Conversation

@JohnOnLee
Copy link
Contributor

@JohnOnLee JohnOnLee commented Feb 26, 2020

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

When we set type options of float for mssql, existing code blindly set it to precision 30, scale 15.
This could cause an issue with a large float value e.g. 43069.0122916667.
see issue tediousjs/tedious#1058
and #11959
to minimize the impact of the issue, we should at least set the scale correctly.

@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #11962 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11962      +/-   ##
==========================================
+ Coverage   96.23%   96.24%   +<.01%     
==========================================
  Files          95       95              
  Lines        9202     9206       +4     
==========================================
+ Hits         8856     8860       +4     
  Misses        346      346
Impacted Files Coverage Δ
lib/dialects/mssql/query.js 95.62% <100%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc836ba...f6e68c8. Read the comment docs.

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.

4 participants