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

Formatting changes from codegen #790

Merged
merged 10 commits into from
May 30, 2019
Merged

Formatting changes from codegen #790

merged 10 commits into from
May 30, 2019

Conversation

rattrayalex-stripe
Copy link
Contributor

I don't think this will be quiiite the last PR of this nature, but this should be the main chunk of work and should probably be reviewed by itself.

Primary categories of changes here:

  1. Ignoring the new line-length rubocop in the directory with the codegen output.
  2. Pulling the object_classes map and resources requires out into their own files, so they can be more easily codegen'd (though I have not yet implemented this).
  3. Reordering include/excludes. TBH I think this ordering is worse, I hope to make it better later. Let me know if you think it's worth investing in now.
  4. Reordering methods – mostly, things that are codegen'd come first, then things that are hardcoded.
  5. Whitespace changes
  6. Moving comments around for convenience
  7. Going from initialize_from to Util.convert_to_stripe_object in most cases – I think this was something you recommended a while ago

r? @ob-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.

LGTM minus one comment. Will ask @brandur-stripe to take a look too just in case.

lib/stripe/object_types.rb Outdated Show resolved Hide resolved
@ob-stripe
Copy link
Contributor

ptal @brandur-stripe

Copy link
Contributor

@brandur-stripe brandur-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 a few comments, but looks mostly good to me as well!

ptal @rattrayalex-stripe

.rubocop.yml Outdated Show resolved Hide resolved
lib/stripe/apple_pay_domain.rb Outdated Show resolved Hide resolved
lib/stripe/bitcoin_receiver.rb Outdated Show resolved Hide resolved
lib/stripe/object_types.rb Outdated Show resolved Hide resolved
@rattrayalex-stripe
Copy link
Contributor Author

Okay, updated to incorporate feedback, and to move resources into their own dir

@rattrayalex-stripe
Copy link
Contributor Author

oops, forgot to
r? @brandur-stripe

Copy link
Contributor

@brandur-stripe brandur-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 Alex! LGTM.

@AnotherJoSmith
Copy link
Contributor

@brandonl-stripe @rattrayalex-stripe There seems to have been a pretty important change in this PR in ea736eb.

Previously when calling any of the methods that used initialize_from, the instance you called the method on would be mutated. When changed to Util. convert_to_stripe_object, we instead return a new instance of the object. This has broken some of our code with PaymentIntents where we would directly return the result of payment_intent.cancel or payment_intent.capture.

If this change in behaviour is intentional, it might be worth it to add it retroactively to the changelog, since it can easily introduce subtle bugs in people's integrations if they're not handling the return values of those methods and are instead expecting the instance to be mutated with the result of the API operation (which is a reasonable expectation IMO).

@rattrayalex-stripe
Copy link
Contributor Author

Thank you very much for reporting @AnotherJoSmith – that was not an intentional change! Thanks very much for reporting. I'll sync with the team on where to go from here.

@rattrayalex-stripe
Copy link
Contributor Author

Alright, #806 should resolve this. I have some work towards a nicer / longer fix on another branch.

Feedback welcome there!

@rattrayalex-stripe
Copy link
Contributor Author

@AnotherJoSmith Sorry for the delayed release – the fix is out in 4.21.2.

@AnotherJoSmith
Copy link
Contributor

Thanks Alex!

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.

5 participants