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

Improve args check to support optional params for makeRequest #827

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

cjavilla-stripe
Copy link
Contributor

@cjavilla-stripe cjavilla-stripe commented Mar 3, 2020

I ran out of time trying to write an automated test for this. I'm not familiar with this nock tooling and was able to get past the root error and my basic test passing locally. Also, didn't see a compact helper so just used this filter.

This is to address #825

(async () => {
  const id = 'acct_1GBs7XDllkSPMYqA';
  const account = await stripe.accounts.retrieve(id)
  console.log(account)
  const account2 = await stripe.accounts.retrieve(id, undefined)
  console.log(account2)
})();

r? @rattrayalex-stripe
cc @stripe/api-libraries

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe left a comment

Choose a reason for hiding this comment

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

Thanks, I think this looks good! I do think we should try to get a test in there for it, added time to pair on this.

A tiny nit is that I much prefer == null for nullish (null/undefined) testing.

ptal @cjavilla-stripe

@cjavilla-stripe cjavilla-stripe force-pushed the cjavilla/undefined-params branch from 968e5a6 to c5414d1 Compare March 5, 2020 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants