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

Add support for Terminal resources #1388

Merged
merged 1 commit into from
Nov 12, 2018
Merged

Add support for Terminal resources #1388

merged 1 commit into from
Nov 12, 2018

Conversation

remi-stripe
Copy link
Contributor

r? @ob-stripe
cc @stripe/api-libraries @stevene-stripe

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

Left one comment, but looks great overall.

Also, could you add the new resources in the StripeObjectConverter dictionary? Not sure if these can appear in webhook events, but better safe than sorry. (Also, I think I forgot to ask you to do the same for the new Radar resources too.)


public override string BasePath => "/terminal/connection_tokens";

public virtual ConnectionToken Create(ConnectionTokenCreateOptions options, RequestOptions requestOptions = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should provide overloads with no options. It seems a little bit silly to force users to create a ConnectionTokenCreateOptions instance that contains nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hear you but then it becomes a mess of overloads and I'm not a big fan, we saw what it did to stripe-java.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tested it, and we can simply set null as the default value for the options argument. It still satisfies the interface and makes the argument optional without requiring an overload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, PTAL

@remi-stripe
Copy link
Contributor Author

@ob-stripe Already did for the StripeObjectConverter part, likely while you reviewed

@remi-stripe remi-stripe merged commit fe8f433 into master Nov 12, 2018
@remi-stripe remi-stripe deleted the remi-add-terminal branch November 12, 2018 15:17
@ob-stripe
Copy link
Contributor

Released as 21.2.0.

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.

2 participants