Add option to set DateFirst #503
Conversation
|
@edwardsmit Not sure if this belongs as a global option. You can set this in your SQL, right? |
|
@tvrprasad Yes you can, but I find that rather |
|
Can you please add an integration test to
test\integration\connection-test.coffee?
…On Wed, Feb 15, 2017 at 2:35 AM, Edward Smit ***@***.***> wrote:
@tvrprasad <https://github.com/tvrprasad> Yes you can, but I find that
rather tedious tiresome.
It's up to the client to set this, SSMS does this for you based on your
locale, so you won't have to set it explicitly in each of your scripts. I
think it's preferable to make sure that your client, once set, uses the
exact desired DateFirst throughout wether you're using a single connection
or create a connection for every call. Now you have to add boilerplate code
where a simple option-config would suffice.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#503 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHml6ROe5LLEHHhUlyDJzsqcYdtjcYAGks5rctUHgaJpZM4L--ym>
.
|
|
@tvrprasad Added the tests |
|
#508 also contains this option, but does not contain the tests from this. |
|
@arthurschreiber Maybe @ariskemper can pull the test into his PR? |
| test.done() | ||
|
|
||
| # Test that the DATEFIRST setting can be changed via an optional configuration | ||
| exports.testDatefirstDefault = (test) -> |
There was a problem hiding this comment.
Test name duplicated. Needs to be renamed, perhaps testDatafirstNotDefault.
And yes, makes sense for @ariskemper to grab this test and merge into his PR.
Thanks for putting this together!
There was a problem hiding this comment.
Since these tests are identical, these can be refactored as below:
function testDateFirstImpl(test, dateFirstSetting)
..... Implementation here. ....
exports.testDateFirstDefault = (test) ->
testDateFirstImpl(test, undefined)
exports.testDateFirstNonDefault = (test) ->
testDateFirstImpl(test, 3)
|
@tvrprasad Refactored tests |
src/connection.js
Outdated
| @@ -126,6 +128,10 @@ class Connection extends EventEmitter { | |||
| this.config.options.database = config.options.database; | |||
| } | |||
|
|
|||
| if (config.options.datefirst != undefined) { | |||
| this.config.options.datefirst = config.options.datefirst; | |||
There was a problem hiding this comment.
Please add a range check here. You can follow the pattern for 'port' config option.
Please add a test. You can follow the pattern for 'badPort' test.
Sorry for not catching this sooner. Thanks.
|
@edwardsmit I just merged changes from #501 by @carlosman for enableArithAbort that includes tests. Please rebase. |
edwardsmit
force-pushed
the
feature/DatefirstConfigurable
branch
from
5c0c6d3
to
94a34ca
Compare
This reverts commit 890cdb9.
edwardsmit
force-pushed
the
feature/DatefirstConfigurable
branch
from
8c00211
to
df61716
Compare
edwardsmit
force-pushed
the
feature/DatefirstConfigurable
branch
from
df61716
to
410cc5c
Compare
|
|
||
| request.on('row', (columns) -> | ||
| dateFirst = columns[0].value | ||
| test.strictEqual(dateFirst, dateFirst || 7) |
There was a problem hiding this comment.
Change variable to name something different than the parameter passed to the function, say datefirstReturned. As it stands, you're comparing the variable to itself.
Nit: I suggest changing parameter name from 'dateFirst' to 'datefirst' for consistency.
|
@tvrprasad Thanks for merging. Do you have a release planned in the near future so that I can use this in my projects? |
|
Stay tuned to this - #477 |
This creeped in PR tediousjs#503. Curiously enough the unit tests added as part of the CL caught the bug when I ran tests on my machine whereas they succeeded on AppVeyor. The difference is that I happened to have a 'port' config value set in my local setup whereas AppVeyor did not. Just a freak coincidence that this would trigger the bug.
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.
DATEFIRSTis set to the default value of7, but there is no way to override this setting.As I'm having the need to set this to an other value I created this PR.
I did not find any rules for PR's, so let me know when you need additional code in order to merge this PR.