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.

Sign up for GitHub

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

Jump to bottom

PHPDoc fixes, CreditNote.previewLines, and autogen prep #837

Merged
merged 1 commit into from
Feb 3, 2020
Merged

PHPDoc fixes, CreditNote.previewLines, and autogen prep #837

merged 1 commit into from
Feb 3, 2020

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Jan 23, 2020 •
edited
Loading

r? @ob
cc @stripe/api-libraries @remi-stripe @cjavilla-stripe @marko-stripe
Changes:

  • Document missing properties on Session, CreditNote, Invoice, InvoiceItem, Subscription, and SubscriptionSchedule.
  • Undocument properties on SubscriptionSchedule that were removed in the latest API versions.
  • Add '.previewLines' method to CreditNote.
  • Adds more specific types for many docstrings. A full list of the PHPDoc changes is here.
  • Several formatting changes/reordering of methods. This is an artifact of autogeneration.

@richardm-stripe richardm-stripe force-pushed the richardm-autogen branch 3 times, most recently from add478d to ca12071 Compare January 23, 2020 20:31
richardm-stripe
richardm-stripe commented Jan 23, 2020
View reviewed changes
lib/EphemeralKey.php Outdated Show resolved Hide resolved
richardm-stripe
richardm-stripe commented Jan 23, 2020
View reviewed changes
lib/Invoice.php Outdated Show resolved Hide resolved
richardm-stripe
richardm-stripe commented Jan 23, 2020
View reviewed changes
lib/InvoiceItem.php Outdated Show resolved Hide resolved
richardm-stripe
richardm-stripe commented Jan 23, 2020
View reviewed changes
lib/Source.php Outdated Show resolved Hide resolved
richardm-stripe
richardm-stripe commented Jan 23, 2020
View reviewed changes
lib/Mandate.php Outdated Show resolved Hide resolved
@richardm-stripe richardm-stripe changed the title Autogen PHPDoc fixes, CreditNote.previewLines, and autogen prep Jan 24, 2020
remi-stripe
remi-stripe requested changes Jan 24, 2020
View reviewed changes
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.

Amazing work, this is so exciting! I left a lot of nitpicky comments and some real blockers.

Also flaggign that I think we have the same problem as with other codegen-ed resources: we never touch the nested resources like CustomerBalanceTransaction and TaxId. I could be wrong, but if I'm correct, if you removed all resources files, then re-codegen you should see all the missing files. It's okay-ish if so for now and something we should fix across all codegen soon

lib/Account.php Outdated
@@ -305,6 +358,7 @@ public function persons($params = null, $opts = null)
return $obj;
}


Copy link
Contributor

Choose a reason for hiding this comment

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

extra new line

lib/Account.php
}
}
return $updateArr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if we could easily put the override like that at the end of the file it'd be great

lib/ApplicationFee.php
@@ -30,6 +30,7 @@ class ApplicationFee extends ApiResource
use ApiOperations\NestedResource;
use ApiOperations\Retrieve;


Copy link
Contributor

Choose a reason for hiding this comment

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

extra new line (won't flag others)

lib/CreditNote.php Outdated Show resolved Hide resolved
lib/Customer.php Outdated Show resolved Hide resolved
lib/Review.php Outdated Show resolved Hide resolved
lib/SetupIntent.php Outdated
@@ -13,14 +13,14 @@
* @property int $created
* @property string|null $customer
* @property string|null $description
* @property mixed|null $last_setup_error
* @property \Stripe\StripeObject|null $last_setup_error
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is an Error object today not sure if we care

lib/Source.php Outdated Show resolved Hide resolved
lib/Source.php Outdated Show resolved Hide resolved
lib/SubscriptionSchedule.php Outdated Show resolved Hide resolved
@marko-stripe
Copy link

Also flagging that I think we have the same problem as with other codegen-ed resources: we never touch the nested resources like CustomerBalanceTransaction and TaxId. I could be wrong, but if I'm correct, if you removed all resources files, then re-codegen you should see all the missing files. It's okay-ish if so for now and something we should fix across all codegen soon

I think that's right. Brian and Josh from Slack saw that issue when they were starting to generate stripe-hack based on stripe-php. They started to also include nested resources when generating the files and it looked like it worked well.

@ob-stripe ob-stripe mentioned this pull request Jan 29, 2020
Merged
@ob-stripe ob-stripe mentioned this pull request Jan 29, 2020
Closed
@ob-stripe ob-stripe force-pushed the richardm-autogen branch 3 times, most recently from 1555861 to 5dbc6b1 Compare January 30, 2020 00:07
@ob-stripe ob-stripe mentioned this pull request Jan 30, 2020
Merged
@ob-stripe ob-stripe force-pushed the richardm-autogen branch 3 times, most recently from 9ac8a52 to cb5ba60 Compare January 31, 2020 18:09
@ob-stripe
Copy link
Contributor

Rebased on latest master and regenerated. I think the remaining diff is as small as it's going to get.

Changes are:

  • path constants for nested resources are moved next to the method definitions for nested resources
  • some changes in whitespace and method definition order due to overrides (we don't have fine grained control over where code snippets manually defined via overrides will end up)

@ob-stripe
Copy link
Contributor

r? @cjavilla-stripe @remi-stripe @richardm-stripe

Just in case I missed anything, but I think we're ready to make the switch to codegen!

remi-stripe
remi-stripe approved these changes Jan 31, 2020
View reviewed changes
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.

OMG it's happening.

@richardm-stripe
Copy link
Contributor Author

lgtm

@cjavilla-stripe
Copy link
Contributor

I think we're good to go. Hard to see the blind spots but excited to see this. Awesome work @richardm-stripe, @ob-stripe, and @remi-stripe!!

@richardm-stripe richardm-stripe merged commit 11a7a9e into master Feb 3, 2020
@richardm-stripe richardm-stripe deleted the richardm-autogen branch February 3, 2020 16:14
@ob-stripe
Copy link
Contributor

Released as 7.23.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remi-stripe remi-stripe remi-stripe approved these changes

Assignees

@richardm-stripe richardm-stripe

Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@richardm-stripe @marko-stripe @ob-stripe @cjavilla-stripe @remi-stripe @stripe-ci