Skip to content

respect customer args #134

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

Closed
wants to merge 2 commits into from
Closed

Conversation

sherrardb
Copy link
Collaborator

addresses #94

Comment on lines 207 to 208
$customer = $self->post_customer( %$customer );
$customer_id = $customer->id;
Copy link
Collaborator

@andrewsolomon andrewsolomon Dec 23, 2019

Choose a reason for hiding this comment

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

I always worry when I see a subroutine's parameters being modified - $customer in this case.

How about this?

my $customer_obj = $self->post_customer( %$customer );
$customer_id = $customer_obj->id;

@sherrardb
Copy link
Collaborator Author

sherrardb commented Dec 23, 2019

i am quite happy with that, and i wish i had come up with it. :-)

as you will see, in this and other upcoming PRs, i am quite keen to move away from an idiom that is used quite frequently in the current codebase.

if (ref($customer)) {
    $customer = $customer->id;
}

which the code you reference is mostly a copy of.

 * use non-destructive idiom
@sherrardb sherrardb mentioned this pull request Dec 24, 2019
Copy link
Collaborator

@andrewsolomon andrewsolomon left a comment

Choose a reason for hiding this comment

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

Looking good!

@sherrardb
Copy link
Collaborator Author

fixed in #147 by removing unsupported argument types

@sherrardb sherrardb closed this Jan 22, 2020
@sherrardb sherrardb deleted the respect-customer-args branch January 22, 2020 02:11
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.

2 participants