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

Stringify data on error reply #23

Closed

Conversation

mlschuh
Copy link

@mlschuh mlschuh commented Jun 27, 2017

When I was trying to debug a validation error, I was receiving the not terribly useful message:

  data: { description: 'Validation failed', errors: [ [Object] ] }

I was able to resolve with a simple stringify on the data and now get something like this:

  data: '{"description":"Validation failed","errors":[{"field":"type","message":"It should be one of these values: \'blah1,blah2,blah3\'","code":"missing_field"}]}',

which is much more useful. I don't expect the testing would need to be updated for this to pass.

@maxkoryukov
Copy link
Contributor

@mlschuh, thanks for your contribution!

Plz, tell, what kind of debugging tool do you use? Did you try the other tool?

The thing is this field error.data is intended not only for your debugger but for many other tools. For example, I've been using this library in my own project, and there I've generated a user-friendly message from the error.data. For such cases, it is more convenient to have an object and not a json-string (which need to be parsed).

You could take a look at these libraries:

PS: console.log is not the best tool for debugging purposes.

@maxkoryukov maxkoryukov self-assigned this Jun 28, 2017
@maxkoryukov maxkoryukov self-requested a review June 28, 2017 20:05
@mlschuh
Copy link
Author

mlschuh commented Jun 29, 2017

Going to issue another PR that does type checking on the result and only stringify the data if it's an object.

@mlschuh mlschuh closed this Jun 29, 2017
@mlschuh mlschuh deleted the validation-error-verbosity branch June 29, 2017 13:04
@mlschuh
Copy link
Author

mlschuh commented Jun 29, 2017

@maxkoryukov Apologies, I didn't see your reply!

I completely understand wanting using additional libraries for the debugging. My goal using this was to just dump a whole bunch of data into FreskDesk from our previous ticket system and thus wanted to eek by with as minimal code as possible. I'll follow your advice and use a library that's better at parsing those in the future. My own personal projects always pass around objects as well, I just never hit a problem with console.log before when doing basic debugging.

Thanks for making me feel welcomed. Never did a PR in github before so it's good to see a nice community.

@maxkoryukov
Copy link
Contributor

@mlschuh , about this:

Going to issue another PR that does type checking on the result and only stringify the data if it's an object.

There is a great GH feature: if you have an opened PR, then ALL your commits to the source branch (in your case mlschuh:validation-error-verbosity) will be included to the opened PR automagically 😉

So, if you have opened a PR, and you want to modify something — you don't need to close/open new PR, just commit/push your changes to the existing PR

@maxkoryukov
Copy link
Contributor

@mlschuh And thank you for your contribution again;)

I will be glad to merge your contributions, take a loot at these things:

I have added Roles stuff yesterday, it is simple: a3dca1b

Also, you could omit changes in the CHANGELOG - I will add them on release

@maxkoryukov maxkoryukov removed the WIP label Jun 29, 2017
@@ -34,7 +34,7 @@ function FreshdeskError(message, data, res) {
this.message = message || 'Error in Freshdesk\'s client API'
this.stack = (new Error()).stack

this.data = data
this.data = JSON.stringify(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, this will cause a lot of errors in other user's code.

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

2 participants