Add additional config options enableQuotedIdentifier, enableArithAbor… #508
Conversation
src/connection.js
Outdated
|
|
||
| class Connection extends EventEmitter { | ||
| constructor(config) { | ||
| super(); | ||
|
|
||
| var self = this; |
There was a problem hiding this comment.
Is there a reason why you add this self variable here? Couldn't we just use this?
There was a problem hiding this comment.
We could use this, since we dont use this in some loop and constructor is not called offten.
There was a problem hiding this comment.
We could use this, since we dont use this in some loop and constructor is not called offten.
Using this in loops is not an issue - I think you're thinking about callbacks passed to other functions. Luckily, tedious is transpiled from modern JavaScript, so you we use => functions in cases where we want to have correct this bindings in callbacks.
I'd like us to follow modern javascript guidelines, so patterns like var self = this are discouraged.
src/connection.js
Outdated
| 'set transaction isolation level ' + (this.getIsolationLevelText(this.config.options.connectionIsolationLevel)) + '\n ' + | ||
| 'set xact_abort ' + xact_abort; | ||
|
|
||
| console.log(ret); |
There was a problem hiding this comment.
This console.log needs to be removed. 😄
src/connection.js
Outdated
| const enableImplicitTransactions = this.config.options.enableImplicitTransactions ? 'on' : 'off'; | ||
| const xact_abort = this.config.options.abortTransactionOnError ? 'on' : 'off'; | ||
|
|
||
| var ret = 'set textsize ' + this.config.options.textsize + '\n' + |
There was a problem hiding this comment.
Can you make this variable const here? Or remove the variable completely and just change it back to a return.
You could also change this string to make use of template literals to make it a bit more readable if you want.
There was a problem hiding this comment.
Template literals are good so lets use it here 👍
src/connection.js
Outdated
| } | ||
| }; | ||
|
|
||
| self.config = _.merge(defaults, config); |
There was a problem hiding this comment.
Can you change this back to not use _.merge? The idea here is to make sure that the connection configuration will never end up with additional config properties other than what is supported.
We had similar code as above before, but that lead us to not have a single, definitive list of supported options. See the discussion in #445.
There was a problem hiding this comment.
This could be done with list of available config options and, if config option is not in the list throwing notice?
|
@ariskemper Can you please add integration tests to connection-test.coffee for these changes? For dateFirst, you can grab tests from #503. |
|
@tvrprasad i will add integration tests for all additional options, also #503 |
Thanks @ariskemper ! |
|
@ariskemper Updated the tests in #503 |
|
@ariskemper I just merged changes from #501 by @carlosman for enableArithAbort that includes tests. Please rebase. |
|
@ariskemper Please pick #503 also. |
|
@arthurschreiber @tvrprasad @edwardsmit i made requested changes with tests. Test for enableAnsiNullDefault and enableAnsiNull is still missing. |
|
I think you need to rebase. Right now your PR is showing a bunch of commits from others. |
ariskemper
force-pushed
the
additional-config-options
branch
2 times, most recently
from
07c292b
to
7631d6a
Compare
…t, enableNumericRoundabort, enableAnsiWarnings, enableAnsiPadding, enableAnsiNull, enableAnsiNullDefault, enableConcatNullYeldsNull,enableCursorCloseOnCommit, enableImplicitTransactions,dateFirst, language, dateFormat.
ariskemper
force-pushed
the
additional-config-options
branch
from
7631d6a
to
e308a29
Compare
|
@ariskemper There are a lot of indentation changes that creeped into this PR that make it difficult to review. Can you please clean this up so that the only changes are the ones relevant to the core intent of this PR. If you're trying to fix indentation, you can prepare a different PR for that. Thanks! |
…t, enableNumericRoundabort, enableAnsiWarnings, enableAnsiPadding, enableAnsiNull, enableAnsiNullDefault, enableConcatNullYeldsNull,enableCursorCloseOnCommit, enableImplicitTransactions,dateFirst, language, dateFormat.
|
@tvrprasad sorry i used github web editor for resolving conflict, which is not very good idea. I rebased with master and resolved conflicts. I think, it could easy be merged or you could use cherry pick to pick up specific commit. |
|
@ariskemper Are you still working on this? |
|
@edwardsmit @tvrprasad @arthurschreiber Would be great, if maintainer would pick the changes and merge it to release, since i currently don't have time to work on this. |
I'll take a crack at this. |
dateFormat, enableAnsiNull, enableAnsiPadding, enableAnsiWarnings, enableConcatNullYeldsNull, enableCursorCloseOnCommit, enableImplicitTransactions, enableNumericRoundabort, enableQuotedIdentifier, language. Relates to tediousjs#507 tediousjs#506 tediousjs#501 tediousjs#503 This is a cleaned up merged version of tediousjs#508.
|
Pushed a cleaned up PR (#580) attributed to @ariskemper |
Add additional config options enableQuotedIdentifier, enableArithAbort, enableNumericRoundabort, enableAnsiWarnings, enableAnsiPadding, enableAnsiNull, enableAnsiNullDefault, enableConcatNullYeldsNull,enableCursorCloseOnCommit, enableImplicitTransactions,dateFirst, language, dateFormat.
Relates to #507 #506 #501 #503