-
Notifications
You must be signed in to change notification settings - Fork 4
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
CL-7765 Updated the pay-client code to handle the callback & un-promised rejections #33
base: master
Are you sure you want to change the base?
Conversation
tusharpari
commented
Feb 24, 2021
•
edited
Loading
edited
- Handled the callback param in request method
- Added try catch block to handle the un-promised rejections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests to cover the next callback functionality that's been added. Additionally, there don't appear to be any Travis CI steps in place. I would like you to confirm that all tests are passing once the above test has been added. Thank you.
src/PayClient.ts
Outdated
: _.get(response, 'body'), | ||
} | ||
) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this else
and just have the return
statement. Many style guides prefer this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as matt said
@aakolkarclassy I've been trying to resolve the bug we discussed the other day as your latest commit will introduce errors later (the value of Tushar had said there was a need to supply a callback to the Unless someone knows why we need this, I'd like to decline this PR. |
all I know it was needed for classyql for the callback |
Please close in favor of https://github.com/classy-org/classyql/pull/435 |