Skip to content
This repository was archived by the owner on Jan 30, 2023. It is now read-only.

Conversation

@bendemboski
Copy link

There may be cases where applications want access to the status code that caused the error (e.g. logging/metrics, or terse user-facing error messages), so this allows them to easily get at it.

My use case is two-fold:

  1. I use mixpanel to log metrics about failed requests that include the status code as a property
  2. When I encounter an unhandled error, I don't want to try to communicate the distinction between "bad request", "server error", etc. -- I just want to show them a generic "Oops, my bad" message with the status code in parentheses so we have something to go on if they contact support.

Given how difficult it is to recover the status code from an error object (especially if it's just an AjaxError -- then the only way is to run a regex on the message ☹️ ), this seems like something worth including in the base implementation and not requiring users to subclass the service in order to get.

There may be cases where callers want access to the status code that caused the error (e.g. logging/metrics, or terse user-facing error messages), so this allows them to easily get at it.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5a57ca0 on bendemboski:expose-error-codes into 528e843 on ember-cli:master.

@alexlafroscia alexlafroscia self-requested a review April 14, 2017 18:33
Copy link
Collaborator

@alexlafroscia alexlafroscia left a comment

Choose a reason for hiding this comment

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

Definitely on board for these changes. I feel like this status code should be part of the error classes itself rather than attaching it before returning the error.

I've been wanting to refactor this bit of error-handling for a while and maybe this is a good opportunity to do that. Would be alright making a few more changes to clean this all up a little?

I'd like to

  • Pull the whole big error-class-decision bit out into a separate function
  • Use a switch statement to determine the right class to use for the error
  • Either pass the status code into the constructor, or have the classes hard-coded with the corresponding error code

Thoughts?

@bendemboski
Copy link
Author

Yeah, that all definitely makes sense to me. I think I'll have some time this weekend to attack this (or if you have it all in your head and want to take it over yourself, I won't be put off or anything).

@alexlafroscia
Copy link
Collaborator

Sweet. Give it a shot if you're able, I've got some other OSS stuff on my plate for the weekend.

@Kilowhisky
Copy link

+1 the new error response types hides status codes. Doing type checking is excessive for unnecessary reasons.

@spruce spruce mentioned this pull request Oct 16, 2017
@alexlafroscia
Copy link
Collaborator

#318 was just merged that handles this change, so I'm going to go and close this. Thanks for the PR though!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants