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

Friendly Errors #3930

Merged
merged 1 commit into from
Dec 9, 2015
Merged

Friendly Errors #3930

merged 1 commit into from
Dec 9, 2015

Conversation

nikz
Copy link
Contributor

@nikz nikz commented Nov 13, 2015

This is an initial PR to iterate on "friendly" (more detailed) error messages for Ember Data, as per RFC101

(emberjs/rfcs#101)

@nikz
Copy link
Contributor Author

nikz commented Nov 13, 2015

@bmac this is a very early cut (missing tests entirely for instance!) but I'm wondering how you feel about this approach?

Hopefully this is enough to iterate on though (I'm working on adding tests etc so feel free to just comment and I'll handle the code part, I'm sure you're plenty busy enough already!)

Thanks again!

@tomdale
Copy link
Member

tomdale commented Dec 2, 2015

This is awesome.

@nikz
Copy link
Contributor Author

nikz commented Dec 2, 2015

Aww shucks 😊

Is there anything more I can do that's useful? Or is it looking OK?
On Wed, 2 Dec 2015 at 18:31, Tom Dale notifications@github.com wrote:

This is awesome.


Reply to this email directly or view it on GitHub
#3930 (comment).

@fivetanley
Copy link
Member

@nikz We are big fans of this commit. However, we would like to add some tests to this so we don't regress this functionality in the future. I am happy to help if you need help getting started writing a test for this.

@tchak Can you give this and the RFC a glance over?

@nikz
Copy link
Contributor Author

nikz commented Dec 2, 2015

Oh yes! Still missing tests, of course - I just wanted to check the
implementation wasn't completely off base.

Tests incoming. I'll sing out if I hit issues. Thanks!
On Wed, 2 Dec 2015 at 18:33, Stanley Stuart notifications@github.com
wrote:

@nikz https://github.com/nikz We are a big fan of this commit. However,
we would like to add some tests to this so we don't regress this
functionality in the future. I am happy to help if you need help getting
started writing a test for this.

@tchak https://github.com/tchak Can you give this and the RFC a glance
over?


Reply to this email directly or view it on GitHub
#3930 (comment).

@tchak
Copy link
Member

tchak commented Dec 3, 2015

LGTM 👍

@nikz
Copy link
Contributor Author

nikz commented Dec 4, 2015

@fivetanley @tchak have added tests now, Travis seems to be having issues at the mo but they run in the browser for me locally :D

@bmac
Copy link
Member

bmac commented Dec 4, 2015

@nikz Do you mind squashing this pr into 1 commit?

@@ -0,0 +1,52 @@
import setupStore from 'dummy/tests/helpers/store';
/* import Ember from 'ember'; */
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this line?

@nikz
Copy link
Contributor Author

nikz commented Dec 4, 2015

@bmac of course, sorry, how messy of me!

🛀 Cleaned that up now :)

EDIT: Apart from that nasty merge commit arghhhh.

@nikz
Copy link
Contributor Author

nikz commented Dec 4, 2015

@bmac there we go, one single commit that's fast-forward on current master :)

bmac added a commit that referenced this pull request Dec 9, 2015
@bmac bmac merged commit c92b4f8 into emberjs:master Dec 9, 2015
@bmac
Copy link
Member

bmac commented Dec 9, 2015

Thanks @nikz

@nikz
Copy link
Contributor Author

nikz commented Dec 9, 2015

Yay! Thanks team 🙌
On Wed, 9 Dec 2015 at 19:23, Brendan McLoughlin notifications@github.com
wrote:

Thanks @nikz https://github.com/nikz


Reply to this email directly or view it on GitHub
#3930 (comment).

@param {Number} status
@param {Object} headers
@param {Object} payload
@return {Object} request information
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a param, and the return a string.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Wanna submit a PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

Choose a reason for hiding this comment

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

jcope2013 added a commit to jcope2013/active-model-adapter that referenced this pull request Apr 21, 2016
…nging in RESTAdapter's handleResponse function emberjs/data#3930, pass along requestData as argument in active-model-adapter handleResponse override method as well as fix handleResponse - returns ajax response if not 422 response test
jcope2013 added a commit to jcope2013/active-model-adapter that referenced this pull request Apr 21, 2016
…ng in RESTAdapter's handleResponse function emberjs/data#3930, pass along requestData as argument in active-model-adapter handleResponse override method as well as fix handleResponse - returns ajax response if not 422 response test
fivetanley pushed a commit to adopted-ember-addons/active-model-adapter that referenced this pull request May 24, 2016
…ng in RESTAdapter's handleResponse function emberjs/data#3930, pass along requestData as argument in active-model-adapter handleResponse override method as well as fix handleResponse - returns ajax response if not 422 response test (#83)
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.

None yet

8 participants