Added decimal variable precision / scale support #2353
Right now knex passes the default values of 8 and 2 to the field. This pull request exposes the variable precision / scale functionality for postgres when there is no precision / scale provided for the column.
Let me know if I need to change anything or if there is a better way to add support for that.
elhigu left a comment
Thanks for PR this looks like a reasonable thing to support.
To keep backwards compatibility it would be better to implement this by passing options object as a parameter. Like
UPDATE: PR is updated, see code + documentation for implementation.
Thank you for the feedback @elhigu !
Just for clarification: The
Without precision and scale:
With precision + scale:
What about precision only?
As per SQL specification,
This does not seem so clean having one argument with different input types and different behavior. Is that implemented for any other field type like that? I couldn't find any by quickly looking through documentation. Or should we switch the configuration to a single options argument only for the decimal field? This would definitely break backwards compatibility plus or would have to add type checks etc.
I think the cleanest solution would be as follows:
The dialects that don't support that could fallback to the current behavior
Per SQL specification, this should result in a scale of 0. Currently scale would be 2 with knex. We could optionally clean this up if we change the functionality.
Let me know how we should proceed here. I am slightly in favor of the approach as it is implemented in this PR, since it would not really break any code besides changing the column type plus it exposes the additional functionality, is closer to the SQL standard and does not introduce additional complexity (different argument type cases).
@elhigu I changed the implementation to be fully backwards compatible. For dialects that don't support the functionality, an exception is raised. Unit-Tests were added for all dialects and documentation is updated.
Let me know if I need to change anything.
Still hacky, but its fine with passing the null :) Sorry for not answering this earlier.
I've been really concerned about backwards compatibility specially with schema methods, because changing them will mean, that people who has done migrations to DB would need to remember to fix all their old migration scripts to be compatible with new format to make for example test code to behave the same way with old installed system, where migrations has already been applied.
I that other backwards incompatible changes also require changing code, but this kind of changes are usually even more hard to spot and can cause really subtle failures. Specially, because we couldn't have added any warnings / deprecation messages to the old plain
We've been trying to get rid of bunch of magic parameters lately, this is not the first API where options object has been suggested instead of list of vague arguments. Schema.timestamps method is good example of that (though it isn't fixed either yet IIRC).
Thanks for the quick merge!
I know, not the cleanest, but that was the best I could come up with without breaking backwards compatibility.
Thanks for keeping the quality up! This library here has helped me a lot!