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

Idea: Add status constants to all Stripe objects. #641

Closed
wants to merge 1 commit into from

Conversation

frewsxcv
Copy link
Contributor

When working with Stripe objects and Stripe Events, it's very common to have conditionals branching on the status of the Stripe object.

Instead of having magic strings throughout our codebase of the various Stripe object statuses, we have string constants defined. As an example for Stripe::Payout:

# https://stripe.com/docs/api#payout_object-status
STRIPE_PAYOUT_STATUSES = [
  STRIPE_PAYOUT_STATUS_PAID = 'paid'.freeze,
  STRIPE_PAYOUT_STATUS_PENDING = 'pending'.freeze,
  STRIPE_PAYOUT_STATUS_IN_TRANSIT = 'in_transit'.freeze,
  STRIPE_PAYOUT_STATUS_CANCELED = 'canceled'.freeze,
  STRIPE_PAYOUT_STATUS_FAILED = 'failed'.freeze,
].freeze

To me, it makes sense for these strings to live in the stripe-ruby codebase so they can be used by other projects too. Since we're working with Ruby, we could just reopen the Stripe namespaces and monkey-patch these in, but I try to avoid doing that wherever I can.

I understand there's an extra maintenance cost for having the statuses live in this project, so no hard feelings if y'all reject it. ✌️

This pull request so far is just for Stripe::Payout, but I'd be willing to add in the rest if this idea is approved! 💁‍♂️

@frewsxcv
Copy link
Contributor Author

@frewsxcv frewsxcv force-pushed the frewsxcv-statuses branch from f2891ca to 6592c0e Compare April 19, 2018 05:16
@brandur-stripe
Copy link
Contributor

Thanks for the PR!

And wow, I have to admit that in 5+ years of Ruby I've never seen this syntax before! Just to save anyone else from looking it up, this would make individual statuses accessible like Payout:: STATUS_PAID and the entire collection of statuses accessible as Payout:: STRIPE_PAYOUT_STATUSES, which is kind of nice.

And yeah, it's definitely a trade off — maintenance of this type of constant has been a bit of a maintenance hassle in Go just because we don't yet have a great automated way of distributing new values out to the bindings, so we're always handling changes reactively instead of proactively, and usually after users tell us. It's also hard to get a good idea of how many people are using them, but unquestionably some people are.

@remi-stripe Do you have a strong opinion on this?

In terms of the strings — I wonder if we should start using # frozen_string_literal: true so that we don't have to freeze every single string like that. It's been available since Ruby 2.3, and we can only expect it to become more prevalent. I just tried enabling it in some of the main files and it hasn't caused any failures, but I'm not sure whether there's any more subtle compatibility issues that we should be worrying about.

@kevinelliott
Copy link

I would love to see the magic comment # frozen_string_literal: true throughout the codebase. Most gems are making this move, and it has real benefits to users of 2.5, and upcoming 2.6.

brandur-stripe pushed a commit that referenced this pull request May 10, 2018
Adds the magic `frozen_string_literal: true` comment to every file and
enables a Rubocop rule to make sure that it's always going to be there
going forward as well.

See here for more background [1], but the basic idea is that unlike many
other languages, static strings in code are mutable by default. This has
since been acknowledged as not a particularly good idea, and the
intention is to rectify the mistake when Ruby 3 comes out, where all
string literals will be frozen. The `frozen_string_literal` magic
comment was introduced in Ruby 2.3 as a way of easing the transition,
and allows libraries and projects to freeze their literals in advance.

I don't think this is breaking in any way: it's possible that users
might've been pulling out one of are literals somehow and mutating it,
but that would probably not have been useful for anything and would
certainly not be recommended, so I'm quite comfortable pushing this
change through as a minor version.

As discussed in #641.

[1] https://stackoverflow.com/a/37799399
@brandur-stripe
Copy link
Contributor

(Opened #649 to resolve the frozen_string_literal suggestion.)

Hey @kevinelliott, I'm afraid this probably isn't the answer your wanted to hear, but I had some conversations with a few people internally, and I ran into a little bit of backpressure for adding constants as suggested here.

The main trouble is that we've done this in a few other libraries (stripe-go being an example), and it's turned into a fairly sizable maintenance hassle in that we often find them to be out of date as changes are made to the enums. When a user finds a set that are not correct it's a particularly poor experience because their assumption would of course be that our official libraries would be correct.

Not to say that this wouldn't be a huge usability improvement — we'd love to have them in here, but we want to find a more sustainable way of doing so (through either code generating or automatic checks against the OpenAPI schema or the like).

Thanks again for the contribution though! Please keep them coming.

@frewsxcv frewsxcv closed this May 22, 2018
@frewsxcv frewsxcv deleted the frewsxcv-statuses branch May 22, 2018 12:03
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.

3 participants