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

Add warning when using .returning() in sqlite3. Fixes #1660 #2471

Merged
merged 1 commit into from Feb 15, 2018

Conversation

Projects
None yet
4 participants
@wubzz
Collaborator

wubzz commented Feb 15, 2018

No description provided.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 15, 2018

hmm.. for some reason some tape oracle tests are running with older nodes, which shouldn't even test for oracle (node-oracledb driver stopped providing binaries for older node versions). I've never seen those fails before... trying to rerun

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 15, 2018

@elhigu Yeah been trying to rerun these the past two hours. It doesn't make much sense :/

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 15, 2018

Maybe they have again done some changes or something in the driver which breaks the tests.

If you don't have hurry with this. I can figure out the cause of breakage during next week. Probably all the other new PRs will be failing too.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 15, 2018

@elhigu Seems oracledb module requires node 8+ according to their latest change -- this utilizes stream.destroy() which according to node docs is not available until 8+. Very strange how Node 5 is passings tests, tho. Perhaps the event is never emitted there?

I'll leave this PR open, its no rush.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 15, 2018

Node 5 and 7 should not be running oracle tests at all, because driver couldn't be installed there anymore. But 4 and 6 were still supposed to be supported while ago.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 15, 2018

Oracle says that it should support node 4,6,8,9 so that self.destory() sounds like node-oracledb bug. Good catch 👍

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 15, 2018

I created an issue oracle/node-oracledb#847

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 15, 2018

Meanwhile if tests are passing and only error are those oracle ones, we can merge there PRs

@elhigu

elhigu approved these changes Feb 15, 2018

@wubzz wubzz merged commit f1faaf9 into tgriesser:master Feb 15, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@faustbrian

This comment has been minimized.

faustbrian commented Feb 20, 2018

Is there some way to disable this warning? It clutters the terminal quite a bit while seeding and debugging and makes it impossible to read anything else during development.

Currently just commented it out in the node_modules files.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 20, 2018

At least it is possible by not using .returning() with sqlite. Also it can be disabled with grep - stdout. There is huge demand for a feature, which will allow hooking in own logging implementation. Ultimately that will allow to filter all logs outputted by knex.

@faustbrian

This comment has been minimized.

faustbrian commented Feb 20, 2018

Yeah, that would be great to hook something like winston up. Was just wondering why that message spammed my screen as I am not calling that method. (maybe objection or bookshelf do)

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 20, 2018

yeah probably objection and bookshelf are using that and both libs are probably using it correctly 👍

@mceachen

This comment has been minimized.

mceachen commented Feb 26, 2018

I've been burned by both winston and bunyan showstopper bugs--please don't pull either of those in as deps. I don't feel that requiring users to grep -v stdout or stderr is a reasonable solution either.

Would it be OK to only omit the warning once?

Another solution, could you not omit the warning at all if, say, the KNEX_SUPRESS_SQLITE_RETURNING_WARNING env was set, or, perhaps better, something like the useNullAsDefault config flag was added, perhaps?

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 27, 2018

I much rather merge the WIP branch by @tgriesser if he feels that it is done. I don't like adding temporary deviations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment