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

wubzz
Copy link
Member

@wubzz 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
Copy link
Member

elhigu commented Jan 28, 2016

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

@wubzz
Copy link
Member 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.

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

Choose a reason for hiding this comment

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

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

@elhigu
Copy link
Member

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
Copy link
Member 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 knex:master Jan 28, 2016
@marcbachmann
Copy link

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

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

@elhigu
Copy link
Member

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants