-
Notifications
You must be signed in to change notification settings - Fork 850
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
Add first()
and last()
functions to Collection
#948
Add first()
and last()
functions to Collection
#948
Conversation
97e609f
to
e862605
Compare
Hi @baijunyao, thank you for the pull request. I think these methods could be useful, but I suggested some modifications to the docstring to clarify that they operate on the current page, not the collection. Does this make sense to you? |
@richardm-stripe I agree, thank you very much. |
db27b66
to
f6924ca
Compare
Hi @baijunyao, still thinking about this. I'm a little uneasy about using That is, if somebody did
They might expect this to print "Customer 2, Customer 1, Customer 2" but this will actually print "Customer 2, Customer 1, Customer 1". (This is sort of contrived, but a scenario like this could matter especially if some of these calls were hidden inside some user's helper functions). What do you think about implementing as |
You're right, but there may still be problems. public function first()
{
return \count($this->data) > 0 ? $this->data[0] : false;
} $custs = $stripe->customers->all(['limit' => 5]);
unset($collection->data[0]);
var_dump($custs->first()); // Undefined offset: 0 So what do you think of changing to the following way? public function first()
{
foreach ($this->data as $item) {
return $item;
}
return false;
}
public function last()
{
foreach (array_reverse($this->data, true) as $item) {
return $item;
}
return false;
} And If |
Hi @baijunyao, sorry for the delay. I do agree I don't think |
Co-authored-by: richardm-stripe <[email protected]>
Co-authored-by: richardm-stripe <[email protected]>
f6924ca
to
409c8ae
Compare
Never mind. |
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.
LGTM. Thank you @baijunyao
(This was released in v7.44.0. Thank you again @baijunyao for the contribution) |
No description provided.