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

Add support for size attribute for float and decimal types. #277

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

moisesrodriguez
Copy link

I've tested these in this repo with unit tests and it works. However, something I noticed is that even thought in this repo sails-mysql DECIMAL support is there. When I test on my local box with sails, every time I set an attribute to type to DECIMAL by the time it reaches sails-mysql/lib/sql.js the type is STRING instead of DECIMAL. Not sure what is going on there.

DECIMAL type is a must for storing monetary values. Not sure why at least from looking at the docs, it's not supported. Since the floating-point (approximate value) types are FLOAT, REAL, and DOUBLE, which they do some rounding. The fixed-point (exact value) types are INTEGER, SMALLINT, DECIMAL, and NUMERIC

@moisesrodriguez
Copy link
Author

@mikermcneil continuing the conversation from Gitter.

@moisesrodriguez re: setting size for floats -- I agree, nothing wrong with it. But I don't want to add it to core right now.. We'll maintain backwards compat. for the current auto-migration settings, but in Sails 1.0, we're planning on pulling auto-migrations out of Waterline and into Sails as a standalone hook. This way Waterline gets to assume you have a viable schema set up at the physical layer, and it'll allow us to make core a little easier to work with.

Not sure how current auto-migration setting currently works. But I don't think this specific change should break anything of that. If you guys could take a look and if it's something that can be implemented without breaking anything it would be great if this get's merged. If not I'll wait until until it is it's own hook and we can implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant