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

Feature/add new event query error #1163

Merged
merged 4 commits into from Jan 28, 2016

Conversation

Projects
None yet
3 participants
@wubzz
Copy link
Collaborator

wubzz commented Jan 28, 2016

I made these changes back in 2014 (like v0.7?) but it was never merged, unsure why. However, tg and ben both seemed ok with it at the time, so I'm giving it another go. References: #594 #593

Use case example:

knex.on('query-error', function(error, obj) {
appLogger.log(error);
});
@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jan 28, 2016

Looks good, could you add also documentation to index.html for this feature?

@wubzz

This comment has been minimized.

Copy link
Collaborator Author

wubzz commented Jan 28, 2016

@elhigu Certainly. Is that text sufficient enough? Kinda bad with docs..

Also, I changed the text for the "query" error from Useful for logging all -> Useful for logging all queries throughout your application. Noticed it spontaneously, and it seemed odd.

index.html Outdated
@@ -2428,10 +2429,33 @@ <h3 id="Interfaces-Events">Events</h3>
<code class="js">knex.select('*')
.from('users')
.on('query', function(data) {
app.log(data);
app.log(data);

This comment has been minimized.

@elhigu

elhigu Jan 28, 2016

Collaborator

Does example still look correct in browser? looks like indentation was removed here by an accident.

index.html Outdated
})
.then(function() {
// ...
// ...

This comment has been minimized.

@elhigu

elhigu Jan 28, 2016

Collaborator

and here too

index.html Outdated
app.log(error);
})
.then(function() {
// ...

This comment has been minimized.

@elhigu

elhigu Jan 28, 2016

Collaborator

indentation

index.html Outdated
// ...
})
.catch(function(error) {
// Same error object as the query-error event provides.

This comment has been minimized.

@elhigu

elhigu Jan 28, 2016

Collaborator

indentation

throw err
})
}.bind(this))

This comment has been minimized.

@elhigu

elhigu Jan 28, 2016

Collaborator

with an arrow function bind shouldn't be needed so this would look slightly cleaner

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jan 28, 2016

Docs were nice! Looks like there was some unintentional edits, which maybe could be still fixed? Thanks a lot for this pq 👍

@wubzz

This comment has been minimized.

Copy link
Collaborator Author

wubzz commented Jan 28, 2016

Sorry about that, should be fixed now.

elhigu added a commit that referenced this pull request Jan 28, 2016

@elhigu elhigu merged commit 5a94bc9 into tgriesser:master Jan 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marcbachmann

This comment has been minimized.

Copy link

marcbachmann commented May 18, 2016

In the docs, that feature is only mentioned by one short sentence. It's also in the changelog, where I found it. Maybe we can add an example with the global event listener.

@wubzz

This comment has been minimized.

Copy link
Collaborator Author

wubzz commented May 18, 2016

@marcbachmann Not sure what you mean by this. Each event has its own pretty extensive description in the docs, including code examples. Reference

@marcbachmann

This comment has been minimized.

Copy link

marcbachmann commented May 18, 2016

knexInstance.on('query-error') isn't documented.
The example only shows queryInstance.on('query-error')

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented May 19, 2016

@marcbachmann open pull request with documentation fixes if you feel like it 👍

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.