-
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
More nested flags #328
More nested flags #328
Conversation
I'd looked at these when reviewing your other PR to see if there were any that needed to be changed and, like you, I couldn't find a use case where they were required. That said I think it's a reasonable change. I'm going to let it sit until @bhelx is back so he and I can look over it together. |
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.
Couple quick comments though...
lib/recurly/adjustment.php
Outdated
'revenue_schedule_type', 'origin', 'product_code' | ||
'currency', 'unit_amount_in_cents', 'quantity', 'description', | ||
'accounting_code', 'tax_exempt', 'tax_code', 'start_date', 'end_date', | ||
'revenue_schedule_type', 'origin', 'product_code' |
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.
Indenting seems off here.
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.
corrected with next commit 5e4b952
lib/recurly/subscription_addon.php
Outdated
'add_on_type', | ||
'usage_type', | ||
'usage_percentage', | ||
'revenue_schedule_type', |
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.
Yeah I don't think we need these indenting changes either
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.
corrected with next commit as well in 5e4b952
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.
corrected the indentation with this last commit
thanks for you feedback - if you and @bhelx can find one or more suitable test cases let me know and i'll add them |
lib/recurly/adjustment.php
Outdated
'revenue_schedule_type', 'origin', 'product_code' | ||
'currency', 'unit_amount_in_cents', 'quantity', 'description', | ||
'accounting_code', 'tax_exempt', 'tax_code', 'start_date', 'end_date', | ||
'revenue_schedule_type', 'origin', 'product_code' |
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.
corrected with next commit 5e4b952
lib/recurly/subscription_addon.php
Outdated
'add_on_type', | ||
'usage_type', | ||
'usage_percentage', | ||
'revenue_schedule_type', |
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.
corrected with next commit as well in 5e4b952
lib/recurly/subscription_addon.php
Outdated
'add_on_type' => 0, | ||
'usage_type' => 0, | ||
'usage' => 0, | ||
'measured_unit' => 0, |
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.
fixed as well in 5e4b952 - all of these was my IntelliJ based IDE and the extra line continuation indents
Hey @g30rg. I will take a look at this this afternoon. Sorry for the delay. |
No worries, did not mean to hassle you, thanks for keeping me in the loop! |
@g30rg not hassling! We really appreciate your help. Normally we are quicker to respond. Thanks again for taking the time on this! |
I couldn't think about how to explicitly test this but it seems safer to make this change than to not. So I've merged as is. |
@bhelx thanks for getting this merged and taking the time to review - for anything else i happen to find i'll of course stick to the convention of tests first again! |
Similar to pull request #326 I had a look at the remaining
populateXmlDoc
overrides in classes extendingRecurly_Resource
and added the$nested
flag to the correspondingparent::populateXmlDoc(...)
calls.The only problem I'm facing is that I could not come up with a test that fails, due to the entities affected (
Recurly_Adjustment
,Recurly_ShippingAddress
,Recurly_SubscriptionAddOn
) not being mandatory similar toRecurly_BillingInfo
in #326.You might be asking yourself, why even bother then? On the one hand I think it would be good practice to pass the optional flag on to the parent call to not break the substitution principle. On the other hand it would be a change to stay future proof with regards to interface changes from recurly's side (if they ever make any of the above mentioned entities mandatory in an update call similar to the one described in #326)
If you can think of a test case(s) I might have missed to proof the necessity of these changes, please let me know - I'm happy to add them.