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

identityCardType as new ALU request parameter #13

Merged
merged 9 commits into from Apr 3, 2016
Merged

identityCardType as new ALU request parameter #13

merged 9 commits into from Apr 3, 2016

Conversation

ghost
Copy link

@ghost ghost commented Feb 19, 2016

No description provided.

@@ -119,6 +124,16 @@ public function withIdentityCardSeries($identityCardSeries)
}

/**
* @param string $identityCardType
* @return $this
Copy link
Member

Choose a reason for hiding this comment

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

I guess the return should be Billing here

Copy link
Author

Choose a reason for hiding this comment

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

i followed the existing code pattern. The return is the self billing object

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. The existing pattern is bad everywhere. How it is right now works in PHPStorm for autocomplete but other developers might not be that lucky.
I guess it would be better to have a separate task for changing it.
Let's leave it like this for this current pull request.

@drealecs
Copy link
Member

Please make sure the tests are passing.
Also if you think the change it's not covered by any tests, add some to do the job.

@drealecs
Copy link
Member

I don't think there is any reason to have the commit 886eefb. Can we revert it?

silviu somesan added 2 commits February 22, 2016 11:26
@drealecs drealecs merged commit 944b7f6 into PayU-EMEA:master Apr 3, 2016
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.

1 participant