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

@lorefnon
Copy link
Collaborator

@lorefnon lorefnon commented May 5, 2019

No description provided.

Copy link
Member

@elhigu elhigu 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

package.json Outdated
@@ -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",
Copy link
Member

@elhigu elhigu May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@lorefnon lorefnon May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

@elhigu elhigu May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@lorefnon lorefnon May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
);
Copy link
Member

@elhigu elhigu May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, there was a test case 👍

Copy link
Collaborator

@kibertoad kibertoad May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@elhigu elhigu May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

@lorefnon lorefnon commented May 5, 2019

Thanks for taking a look so soon.

@kibertoad kibertoad merged commit e2e044c into knex:master May 19, 2019
1 of 2 checks passed
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants