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

CL-7765 Updated the pay-client code to handle the callback & un-promised rejections #33

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tusharpari
Copy link

@tusharpari tusharpari commented Feb 24, 2021

  1. Handled the callback param in request method
  2. Added try catch block to handle the un-promised rejections

@tusharpari tusharpari self-assigned this Feb 24, 2021
@tusharpari tusharpari requested a review from timothyk7 February 24, 2021 08:55
@tusharpari tusharpari changed the title CL-7765 Updated the pay-client code to handle the un-promised rejections CL-7765 Updated the pay-client code to handle the callback function & un-promised rejections Feb 24, 2021
@tusharpari tusharpari changed the title CL-7765 Updated the pay-client code to handle the callback function & un-promised rejections CL-7765 Updated the pay-client code to handle the callback & un-promised rejections Feb 24, 2021
Copy link

@antigenius antigenius left a 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 {

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.

Choose a reason for hiding this comment

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

Updated the code.

Copy link
Contributor

@timothyk7 timothyk7 left a 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

@antigenius
Copy link

antigenius commented Mar 3, 2021

@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 reponseObj.count is used to iterate across paginated results from the API. However, I'm now not sure this commit is even necessary. I believe @tusharpari is out on leave, but do you or anyone else know why we need this functionality? @timothyk7, do you recall why the callback functionality was needed?

Tushar had said there was a need to supply a callback to the request method in order to migrate from ClassyPay Client to ClassyPay Core. However, the request method is private, and none of the public methods in the class supply a callback. It appears that extra arguments were added to some method signatures and a conditional to use the callback if provided. However, after taking a closer look at the changes, I don't believe any of that code will ever be used.

Unless someone knows why we need this, I'd like to decline this PR.

@timothyk7 timothyk7 closed this Mar 4, 2021
@timothyk7 timothyk7 reopened this Mar 4, 2021
@timothyk7
Copy link
Contributor

@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 reponseObj.count is used to iterate across paginated results from the API. However, I'm now not sure this commit is even necessary. I believe @tusharpari is out on leave, but do you or anyone else know why we need this functionality? @timothyk7, do you recall why the callback functionality was needed?

Tushar had said there was a need to supply a callback to the request method in order to migrate from ClassyPay Client to ClassyPay Core. However, the request method is private, and none of the public methods in the class supply a callback. It appears that extra arguments were added to some method signatures and a conditional to use the callback if provided. However, after taking a closer look at the changes, I don't believe any of that code will ever be used.

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

@antigenius
Copy link

Please close in favor of https://github.com/classy-org/classyql/pull/435

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.

4 participants