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

Diffs from codegen #809

Closed
wants to merge 3 commits into from
Closed

Diffs from codegen #809

wants to merge 3 commits into from

Conversation

rattrayalex-stripe
Copy link
Contributor

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

The biggest difference I can find is that we no longer output |null for optional fields in comments. Should we? I don't think it should be too hard.

There are a few other comment regressions, like

- * @property \Stripe\Collection $reversals
+ * @property mixed $reversals

Feels on how important those are to fix? Should be doable.

@marko-stripe
Copy link

There still is a fix for |null on my computer (@ob-stripe applied that before somehow already) and I also was investigating better types. It's likely something we can get from autogen from stripe-java.

@rattrayalex-stripe
Copy link
Contributor Author

Cool, then a patch would likely be appreciated but I can probably take!

$this->refreshFrom($response, $opts);
return $this;
}
use ApiOperations\NestedResource;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor but I'd recommend having this at the top. It's really easy to miss and it's an important concept core to those classes

@@ -201,6 +256,8 @@ public static function allCapabilities($id, $params = null, $opts = null)
return self::_allNestedResources($id, static::PATH_CAPABILITIES, $params, $opts);
}

const PATH_EXTERNAL_ACCOUNTS = '/external_accounts';
Copy link
Contributor

Choose a reason for hiding this comment

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

This was useful when we were writing code manually. I wonder if it isn't cleaner/easier to just have the route in the function now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stupid PHP question: would removing those constants be a backwards-breaking change? Not sure if they are public or not.

@ob-stripe
Copy link
Contributor

Feels on how important those are to fix?

I think we should definitely fix this. See e.g. this issue: #793. Ideally, we would replace all instances of mixed with the actual type:

  • \Stripe\<Resource> for actual API resources (e.g. \Stripe\Plan for plan objects, etc.)
  • \Stripe\Collection for list objects (unfortunately AFAIK PHPDoc has no support for "generics", so there's no way to do e.g. \Stripe\Collection<\Stripe\Refund> or anything like that)
  • \Stripe\StripeObject for nested hashes that are not API resources
  • array for actual lists

We don't have to fix everything at once, but we should at least not make the current state worse.

@rattrayalex-stripe
Copy link
Contributor Author

Awesome, thanks for the feedback! I think those should all be pretty feasible; hopefully we'll be able to get through them next week.

@ob-stripe
Copy link
Contributor

Closing in favor of #837.

@ob-stripe ob-stripe closed this Jan 29, 2020
@ob-stripe ob-stripe deleted the ralex/codegen-diffs branch January 29, 2020 23:42
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