-
Notifications
You must be signed in to change notification settings - Fork 93
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
Enhancement: Enable IntelliSense (IDE friendly) for class properties #278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about stripping this back to just type and property name? I think that would be pretty straightforward to generate off of the XML. I think the descriptions are going to be tricker to fill in and keep updated etc.
* @property string $invoices The URL of invoices for the specified account. | ||
* @property string $redemption The URL of the coupon redemption for the specified account. | ||
* @property string $subscriptions The URL of subscriptions for the specified account. | ||
* @property string $transactions The URL of transactions for the specified account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these values that are listing as being URLs will actually be present as strings. I think they'll be Stubs for loading the full object.
* class Recurly_Adjustment | ||
* @property string $type The type of adjustment to return: charge or credit. | ||
* @property string $account The URL of the account for the specified adjustment. | ||
* @property string $invoice The URL of the invoice for the specified adjustment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here I think anything with an href=""
in the XML will become a Recurly_Stub
in the object.
* class Recurly_BillingInfo | ||
* @property string $account_code Account's unique code. | ||
* @property string $token_id A token generated by Recurly.js | ||
* @property string $currency Currency in which invoices will be posted. Only applicable if this account is enrolled in a plan has a different currency than your site's default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's definitely more properties on this object: https://dev.recurly.com/docs/create-an-accounts-billing-info-credit-card
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I added the missing fields.
* class Recurly_Plan | ||
* @property string $plan_code Unique code to identify the plan. This code may only contain the following characters: [a-z 0-9 @ - _ .]. Max of 50 characters. | ||
* @property string $name Plan name. Max of 255 characters. | ||
* @property array of objects $unit_amount_in_cents Array of currency objects, see example below. Max 10000000. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this one is showing the limits of the site we use for docs. In PHP land it's a Recurly_CurrencyList
so this makes me wonder if we should just list the properties and a type but not bother with copying over the descriptions.
|
Oh looks like there's a conflict in transaction.php could you rebase this on master? |
Conflicts: lib/recurly/transaction.php
@drewish rebased! Wow what an awful experience.... I'll have to make sure i update my forks before making edits. "This branch has no conflicts with the base branch" |
Fixed some of the types and added a missing * to enable the intellisense in a few files. |
What you've got here looks pretty good to me. You okay if I merge what's here or were you wanting to get other object types? |
Oh one request. Could you make the top of the CHANGELOG read:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Happy to add in other stuff if you'd like to provide them.
Thanks for doing the merge! Just added invoice and subscriptions but maybe we'll do that another day. https://github.com/phpdave/recurly-client-php/blob/master/lib/recurly/invoice.php |
Yeah few free to queue them up and we can do another PR |
Added phpdoc class properties so that intellisense can pick up "magic" fields created by the use of
__get
Also added params and return type for Recurly_Transaction's get() function. This allows the intellisense to know that the returned object is a Recurly_Transaction. Might want to add this to other class methods/functions.