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

Ensure measurement not found returns status code 200 #2001

Merged
merged 2 commits into from
Mar 18, 2015

Conversation

corylanou
Copy link
Contributor

Follow up to #1999

@otoolep
Copy link
Contributor

otoolep commented Mar 18, 2015

All this string checking of errors feel hokey, but it's seems it's what we have right now. If it's works, +1.

@corylanou -- perhaps we need to revisit the errors at some point, to make better use of error types?

@corylanou
Copy link
Contributor Author

@otoolep yes, I'm totally unhappy with this. It's about the most brittle way you can test this, and is likely to bite us again in the future. We probably need a better way of handling errors in the future. I would be a big fan of error types instead of vars as it gives us some more flexibility. However, if you are bubbling up errors, and format them to combine with another error, you will lose the type info anyway. I think more generically we need an error type, that has a message (string), and then a category, and if we want to bubble that up, we only change the message, not the category, or something like that.

toddboom added a commit that referenced this pull request Mar 18, 2015
Ensure measurement not found returns status code 200
@toddboom toddboom merged commit f644ba2 into master Mar 18, 2015
@toddboom toddboom deleted the measurement-not-found-II branch March 18, 2015 18:21
mark-rushakoff pushed a commit that referenced this pull request Jan 11, 2019
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

3 participants