Skip to content

Add createQuery and remove error handler for now - #88

Merged
daffl merged 4 commits into
masterfrom
create-query
Nov 30, 2016
Merged

daffl merged 4 commits into
masterfrom
create-query

Conversation

@daffl

@daffl daffl commented Nov 30, 2016

Copy link
Copy Markdown
Member

Closes #87

@kc-dot-io

Copy link
Copy Markdown
Contributor

👍

@ekryski

ekryski commented Nov 30, 2016

Copy link
Copy Markdown
Contributor

@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

daffl commented Nov 30, 2016

Copy link
Copy Markdown
Member Author

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

ekryski commented Nov 30, 2016

Copy link
Copy Markdown
Contributor

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

daffl commented Nov 30, 2016

Copy link
Copy Markdown
Member Author

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

@ekryski

ekryski commented Nov 30, 2016

Copy link
Copy Markdown
Contributor

: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