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

Unify object options handling for datetime/timestamp across dialects and update type definitions #3181

Merged
merged 3 commits into from May 19, 2019

Conversation

Projects
None yet
3 participants
@lorefnon
Copy link
Contributor

commented May 5, 2019

No description provided.

@lorefnon lorefnon force-pushed the lorefnon:issue#3179 branch 2 times, most recently from 0022684 to 973f0ce May 5, 2019

@elhigu
Copy link
Collaborator

left a comment

This needs to wait a bit for me to get oracle tests back online. I have already working docker setup for xe 18c , but some tests are still failing

@@ -85,7 +85,7 @@
"babel": "rimraf ./lib && babel src --out-dir lib --copy-files",
"coveralls": "nyc report --reporter=text-lcov | coveralls",
"dev": "rimraf ./lib && babel -w src --out-dir lib --copy-files",
"lint": "eslint '{src,test}/**/*.js'",
"lint": "eslint src/**/*.js test/**/*.js",

This comment has been minimized.

Copy link
@elhigu

elhigu May 5, 2019

Collaborator

Probably some quoting is needed to prevent shell from expanding those wildcards on some platforms

This comment has been minimized.

Copy link
@lorefnon

lorefnon May 5, 2019

Author Contributor

Yes you are right. Updated this to use double quotes - which prevent globbing in sh. Single quotes are problematic in cmd.

},

timestamp: function(without) {
return without ? 'timestamp' : 'timestamp with local time zone';
timestamp: function(withoutTz) {

This comment has been minimized.

Copy link
@elhigu

elhigu May 5, 2019

Collaborator

This will need to wait until oracle tests are back online + testcase

This comment has been minimized.

Copy link
@lorefnon

lorefnon May 5, 2019

Author Contributor

Given that the outcome is identical I don't see why it will need to wait, but its up to you.

@@ -669,6 +669,16 @@ describe('OracleDb SchemaBuilder', function() {
expect(tableSql[0].sql).to.equal(
'alter table "users" add "foo" timestamp with local time zone'
);

This comment has been minimized.

Copy link
@elhigu

elhigu May 5, 2019

Collaborator

Cool, there was a test case 👍

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 5, 2019

Collaborator

Considering that there is a unit test for this, and output is same, do we really need to wait before merging?

This comment has been minimized.

Copy link
@elhigu

elhigu May 5, 2019

Collaborator

Yep, I don't believe that test work unless I can see that it has been ran successfully and I don't wan't to check that manually right now.

@lorefnon lorefnon force-pushed the lorefnon:issue#3179 branch from 973f0ce to 6460efb May 5, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Thanks for taking a look so soon.

@kibertoad kibertoad merged commit e2e044c into tgriesser:master May 19, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.006%) to 88.65%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2019

@lorefnon Thanks! Great work as usual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.