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

Add createQuery and remove error handler for now #88

Merged
merged 4 commits into from
Nov 30, 2016
Merged

Conversation

daffl
Copy link
Member

@daffl daffl commented Nov 30, 2016

Closes #87

@kc-dot-io
Copy link
Contributor

👍

@ekryski
Copy link
Member

ekryski commented Nov 30, 2016

@daffl what was the reasoning for removing the error handler? imho it probably should beefed up rather than pulled out. It catches, transforms and re-throws. Maybe I'm missing something...

@daffl
Copy link
Member Author

daffl commented Nov 30, 2016

The reason was that it doesn't seem to do anything. Even when using SQLite none of the errors get converted (and it's not supporting any other DB drivers).

I found it really hard to find all the possible errors the ORMs can throw and map them (I can't find anything about it in the Knex or most of the driver docs). Now that we have error hooks it's pretty easy to just map them to what you need yourself.

@ekryski
Copy link
Member

ekryski commented Nov 30, 2016

Really? The SQLite errors are not getting mapped? Weird. Ya you have to do some digging in the code of the drivers to find the errors that spit out. I'd sort of like to keep it for now if it's not interfering with anything but at the same time if you just ignored me I wouldn't be upset either. 😉

The only thing is this may be a breaking change if the SQLite errors are getting mapped. I could have sworn they were.

@daffl
Copy link
Member Author

daffl commented Nov 30, 2016

Good point. And it doesn't really have anything to do with what this PR is for. I added it back.

@ekryski
Copy link
Member

ekryski commented Nov 30, 2016

:shipit: That is a good point though. We should check to see if this is actually doing stuff and write tests for it and beef it up or just kill it now that we have error hooks.

@daffl daffl merged commit c790cb7 into master Nov 30, 2016
@daffl daffl deleted the create-query branch November 30, 2016 18:37
mcchrish added a commit to feathersjs-ecosystem/feathers-objection that referenced this pull request Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants