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

[wip] Add support for unsettable fields #1956

Closed
wants to merge 1 commit into from
Closed

Conversation

ob-stripe
Copy link
Contributor

No description provided.

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

Added some comment. What does this change look like in VSCode and other IDE for .NET? How confusing/clear are the docstrings/helper?


[JsonProperty("default_source")]
public string DefaultSource { get; set; }
public AnyOf<string, EmptyParam> DefaultSource { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I am fairly sure you can never unset a default source. If the openapi spec says otherwise, the spec is wrong.

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 copied this from stripe-java, so the spec is likely wrong.


[JsonProperty("email")]
public string Email { get; set; }
public AnyOf<string, EmptyParam> Email { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to support this on string since you can simply pass an empty string today and it just works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's certainly debatable. Is this:

var options = new CustomerUpdateOptions { Email = EmptyParam.Empty };

better/clearer than this:

var options = new CustomerUpdateOptions { Email = "" };
// or even
var options = new CustomerUpdateOptions { Email = string.Empty };

?

In stripe-java we do define an overload with EmptyParam even for strings, but we disallow empty strings, which we don't do in stripe-dotnet.

Let's discuss this with @cjavilla-stripe and @richardm-stripe next week.

@clement911
Copy link
Contributor

Will this fix #1954 and allow setting fields to null?

Wouldn't EmptyParam.Empty be better named NullParam.Null since it's setting to null and not empty string?
This will work with other types such as long, DateTime, bool too right? If so, the rename would even make more sense because in .net we talk about a nullable long, not an empty long (for example).

What do you think?

@ob-stripe
Copy link
Contributor Author

Will this fix #1954 and allow setting fields to null?

As long as the field can be unset on the API's side, yes. That's not the case for all fields.

Wouldn't EmptyParam.Empty be better named NullParam.Null since it's setting to null and not empty string?

Technically there isn't really a difference. Because Stripe's API uses form-encoding for requests, there isn't an explicit notion of "null" -- if you unset a field named foo, the encoded request body will look like foo=.

That said that's more of an implementation detail and I agree that EmptyParam.Empty might not be the best name. We'll take this into consideration. Thanks for the feedback!

@clement911
Copy link
Contributor

Technically there isn't really a difference. Because Stripe's API uses form-encoding for requests, there isn't an explicit notion of "null" -- if you unset a field named foo, the encoded request body will look like foo=.

That said that's more of an implementation detail and I agree that EmptyParam.Empty might not be the best name.

Indeed that sounds like an implementation detail.

Consider the fact that if you set foo= and load foo back from the API, the API would return null and not "". If I understand correctly...

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2020

CLA assistant check
All committers have signed the CLA.

@ob-stripe
Copy link
Contributor Author

Closing due to staleness.

@ob-stripe ob-stripe closed this Sep 24, 2020
@ob-stripe ob-stripe deleted the ob-empty-param branch September 24, 2020 16:40
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