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

Use Enums instead of Strings? #1673

Open
richardlawley opened this issue Jun 20, 2019 · 7 comments
Open

Use Enums instead of Strings? #1673

richardlawley opened this issue Jun 20, 2019 · 7 comments
Labels

Comments

@richardlawley
Copy link
Contributor

With some of the more recent changes, there have been a lot of string "status" fields and API methods which ask for a string value, which must be one of a set of values. Could these not be implemented as enums instead of strings?

Examples:

  • List Payment Methods method - has a required type parameter but doesn't tell you what values this can be. API error message helpfully provided card, card_present
  • Payment Intent Object - has a status field, API documentation describes the allowed values
  • AddExpand method - previously we would set ExpandPropertyName = true, which was type-safe. Now we need to know the name of the object to expand, and this doesn't match up with the type names in this SDK (e.g. balance_transaction on charge). I appreciate that you can't easily do multi-level expansion with enums, so perhaps a static class of consts would be helpful in this scenario

Using enums instead of strings would make development a lot easier and reduce the possibility of runtime bugs.

@remi-stripe
Copy link
Contributor

@richardlawley Thanks for the feedback! It's definitely something we've been thinking about recently to better handle enum values across our libraries. We currently define constants in stripe-go and stripe-php for example.

The main downside with enums is that some features are "gated" in the API (accessible to only a subset of accounts) which makes handling those "secret values" with enums a bit trickier in some languages.

Converting (and maintaining) all enums by hand is always a bit tricky. It's easy to miss a new value for example. We're currently working on auto-generating our libraries though which would make this a lot easier. For example, for the past few weeks, stripe-java has been automatically generated from our openapi spec. Doing so has allowed us to automatically generated classes for typed parameters and to include enum values for those too.

We're currrently working on auto-generating other languages (node, python and ruby) but plan to tackle stripe-dotnet later this year!

@ghost
Copy link

ghost commented Jul 11, 2019

Another (temporary) solution might be implicit conversion.

Based on #1675, the StringEnum class could be extended by implicit operators that allow a conversion from/to string. For most of the cases, this would allow API users to use the predefined "fake" enum values, but for those that have access to "gated" API endpoints, this would not stop them from submitting a string.

Assuming that the implicit operators have been defined, both of these calls would be totally valid:

var options = new PaymentIntentUpdateOptions();
options.AddExpand(ExpandEnum.BalanceTransaction);
options.AddExpand("balance_transaction");

The BaseOptions class would change to:

public class BaseOptions : INestedOptions
{
    /// <summary>Specifies which fields in the response should be expanded.</summary>
    [JsonProperty("expand", NullValueHandling=NullValueHandling.Ignore)]
    public List<ExpandEnum> Expand { get; set; }
    
    // ...
}

@granthoff1107
Copy link

granthoff1107 commented Jul 23, 2019

I was just about to open an Issue for this. Its not intuitive to the developer take PaymentIntentService.Cancel(), the cancellationreason is a string, but you cannot provide your own cancellation reason, you must provide one of 4 reasons

@rleeson24
Copy link

If you are using C#, you can use nameof [ex nameof(Stripe.Plan.Product)], theoretically. But I am getting errors that "This property cannot be expanded (Product)" when I attempt it on PlanService.GetAsync, so I can't prove that.

@ob-stripe
Copy link
Contributor

@rleeson24 I think you would have to convert the result from CamelCase to snake_case for the API to accept it. Even then there are cases where expandable fields are not named after the object type (e.g. Charge.OnBehalfOf is an Account) so this wouldn't work everywhere.

@rleeson24
Copy link

You are correct...I looked at the source and saw that it was making it lower and inserting the underscore and I am up and running with the couple instances where I use that. Thanks for the reply!

@iamcarbon
Copy link
Contributor

iamcarbon commented Jun 30, 2023

Rather than using a real enum, would it make sense to introduce a named Choice for known values/ options with an implicit conversion to and from a string? This would make the API source code compatible and a viable option in a major API upgrade and maintain the ability to use custom / private values. It would be binary breaking, however -- so users would need to recompile.

For example:

public abstract Choice(string name) 
{
    public Name { get; } = name;
    
    ...implicit string operators
    .. equality operators
}

With implementations for each? For example:

[method:JsonConverter(typeof(CancellationReasonConverter))]
public sealed CancellationReason(string name) : Option(name)
{
   public static readonly CancellationReason Duplicate = new("Duplicate");
   public static readonly CancellationReason Fraudulent = new("Fraudulent");
   
   // consider implement a factory to avoid allocations when deserializing from JSON and to support reference equaility
   public static CancellationReason Parse(ReadOnlySpan<byte> name) {
       if (name.SequenceEquals("Duplicate"u8) { 
          return CancellationReason.Duplicate;
       }
       
       // throw if the choice does not support custom or private values
       // otherwise, create a custom option
       return CancellationReason(Encoding.Ascii.GetString(name));
   }
   
   public static CancellationReason Parse(ReadOnlySpan<char> name) {
      ...
   }
   
}

PING @remi-stripe @ob-stripe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants