add MSSQL support (requested changes) #33
Conversation
Mssql is now using builder._pushValue for limit/offset Replaced whitspaces with tabs
Replaced spaces with tabs Reverted package.json
This commit refactors the `mssql` dialect as per the feedback from @artzhookov on 2do2go#26. - Include all test files in gulp lint tasks - Move mssql `value` block override to blocks.js - Update mssql `limit` block to use `_.isUndefined` - Clean up minor mssql sql letter case and format - Set the `mssql` `identifier` `prefix` and `suffix` This commit also introduces two changes to the `base` dialect. - Refactor the `base `insert:values` `block` function to clean up the logic - Refactor the `mssql` `inserted:values` `block` function to match the `base` function, and also include `returning` params, as it is needed. (mssql puts `output` keyword before `values` keyword) - Update `buildTemplate` to include an edge case where a `buildBlock` function does not return any value. This prevents unnecessary whitespace in the final query string. These changes are not strictly necessary. The `mssql` `insert:values` `block` could use the same structure as the original `base` `block`, and unnecessary whitespace has no issues in terms of functionality, simply ascetics. This commit refactors the `mssql` tests as per the changes made. Test coverage and layout is not ideal, but functional.
This commit updates the tests for the `mssql` dialect to improve coverage. Additionally, some refactoring is done to the `mssql` dialect to remove unnecessary code.
| module.exports = function(dialect) { | ||
| var parentValueBlock = dialect.blocks.get('value'); | ||
|
|
||
| dialect.blocks.set('value', function(params) { |
There was a problem hiding this comment.
Logic moved from builder._pushValue to blocks.js. number, string, and null/undefined cases are handled by parentValueBlock(params) I belive.
|
|
||
| dialect.blocks.set('limit', function(params) { | ||
| var result = ''; | ||
| if (_.isUndefined(params.offset)) { |
There was a problem hiding this comment.
_.isUndefined used.
| }); | ||
|
|
||
| dialect.blocks.set('offset', function(params) { | ||
| var prefix = (!params.sort) ? 'order by 1 ' : ''; |
There was a problem hiding this comment.
This function was refactored to clean up the program flow. offset case fixed
| }); | ||
|
|
||
| dialect.blocks.set('insert:values', function(params) { | ||
| var values = params.values; |
There was a problem hiding this comment.
This function was refactored a little bit, as was the base blocks.js. This cleans up the values and fields variable assignment.
returning is needed here, because it is used in the next buildTemplate call. {returning} is now part of the insertValues template. See template
#26 (comment)
The unnecessary code was removed
|
Hi @artzhookov, is there anything I can do to help move this pull request along? Any additional testing, cases, or code quality you would like me to address? Thanks for taking the time to look at this, it's a great little library! |
|
Hi @ojourmel, first of all thank you for your help! |
3 participants
Refactor mssql dialect as per pull #26 feedback
This pull request is a continuation of #26. I used @iteufel's work, implemented the changes requested, and improved the test coverage.
valueblock override toblocks.jslimitblock to use_.isUndefinedmssqlidentifierprefixandsuffixThere are two changes to the
basedialect.baseinsert:valuesblockfunction to clean up the logic (match themssqlinserted:valuesblockfunction)buildTemplateto include an edge case where abuildBlockfunction does not return any values. This prevents unnecessary whitespace in the final query string.These changes are not strictly necessary. The
mssqlinsert:valuesblockcould use the same structure as the originalbaseblock, and unnecessary whitespace has no issues in terms of functionality, simply aesthetics.I will self review this pull request to record, and address the feedback given on #26
Let me know if this pull request is appropriate, or if there is anything I can help clean up.
Thanks for this great library!