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.

Sign up for GitHub

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

Jump to bottom

SO-141 Problem+Json for error messages SO-141 #270

Merged
merged 29 commits into from
May 8, 2019
Merged

SO-141 Problem+Json for error messages SO-141 #270

merged 29 commits into from
May 8, 2019

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented May 7, 2019 •
edited
Loading

This PR is a little bit bigger than expected but there were couple of changes that were needed to make the following modifications a little bit easier:

  1. Updated different packages to stay with the latest versions
  2. Removed custom tslint configuration to use the one we have in Stoplight. This, with a tslint --fix has changed a little bit of code to use 120 line width instead of 80 (that's most of the changes)
  3. Removed some casts and any in favour of type guards and unknown
  4. Modified the exception handler for the http server so that either forward the ProblemJsonError exception or if it's not provided, constructs a default problem+json object starting from a generic Error object anyway

#266

@XVincentX XVincentX marked this pull request as ready for review May 7, 2019 13:45
@XVincentX XVincentX changed the title Problem+Json for error messages SO-141 Problem+Json for error messages SO-141 May 7, 2019
@XVincentX XVincentX dismissed stale reviews from philsturgeon via edeed95 May 8, 2019 09:08
chris-miaskowski
chris-miaskowski suggested changes May 8, 2019
View reviewed changes
Copy link
Contributor

@chris-miaskowski chris-miaskowski left a comment

Choose a reason for hiding this comment

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

Had a couple of minor comments. Other than that it looks AMAZING!!!!!111oneoneone :)
Remember to implement the 405 before you close SO-141 :]

@XVincentX
Copy link
Contributor Author

Remember to implement the 405

This is implemented in this PR, see the NO_METHOD_MATCHED_ERROR error

@XVincentX
Copy link
Contributor Author

XVincentX commented May 8, 2019 via email

chris-miaskowski
chris-miaskowski previously approved these changes May 8, 2019
View reviewed changes
Copy link
Contributor

@chris-miaskowski chris-miaskowski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See my last comment.
Besides that it is a FANTASTIC PIECE OF WORK!! SO SPECTACULAR I HAD TO HOLD MY BREATH! :) 🇺🇸

@XVincentX XVincentX force-pushed the feat/problem-json branch 2 times, most recently from 8d1f4e8 to 66eb020 Compare May 8, 2019 12:08
chris-miaskowski
chris-miaskowski approved these changes May 8, 2019
View reviewed changes
Copy link
Contributor

@chris-miaskowski chris-miaskowski left a comment

Choose a reason for hiding this comment

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

:shipit:

@XVincentX XVincentX merged commit a5a3a67 into develop May 8, 2019
@XVincentX XVincentX deleted the feat/problem-json branch May 8, 2019 12:12
@XVincentX XVincentX mentioned this pull request May 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-miaskowski chris-miaskowski chris-miaskowski approved these changes

@philsturgeon philsturgeon Awaiting requested review from philsturgeon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@XVincentX @philsturgeon @chris-miaskowski