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

feat(collections): Returns the index of the last element in the given array matching the given predicate #1062

Merged
merged 3 commits into from
Jul 27, 2021

Conversation

getspooky
Copy link
Contributor

@getspooky getspooky commented Jul 26, 2021

Please see #1060

closes #1060

@kt3k
Copy link
Member

kt3k commented Jul 27, 2021

I think we need a test case of multiple matches like findLastIndexTest([[0, 1, 2, 3, 4, 5, 6], (it) => it % 2 === 0], 6) to confirm the function actually finds the last index.

@getspooky
Copy link
Contributor Author

@kt3k sure, I added multiple matches.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@getspooky Thank you for updating! LGTM

@kt3k kt3k merged commit 2ff5e3c into denoland:main Jul 27, 2021
@getspooky
Copy link
Contributor Author

getspooky commented Jul 27, 2021

@kt3k I forgot to add documentation for findLastIndex. so i opened a PR for that #1066
thanks

@LionC
Copy link
Contributor

LionC commented Jul 27, 2021

I personally would use undefined for no index - I am aware that the native .indexOf uses -1, but all other functions within collections use undefined to communicate that nothing was found, which also has the huge benefit of forcing the consumer of the function to actually handle that case (if they are using typescript), while the current solution looks like it is fine (-1 is a number) but might blow up during runtime.

I will create a separate PR for that change.

@getspooky
Copy link
Contributor Author

Thanks for feedback 👍🏼 ,
As you might know Array.prototype.findIndex() returns -1 indicating that no element passed the test. So i just followed the same convention provided by API.

@LionC
Copy link
Contributor

LionC commented Jul 27, 2021

I see the inspiration, however, collections is its own module and tries to be as Typescript "native" as possible, while the ES spec functions have a very historical API that cannot be changed anymore.

The same applies to filter / map functions, which have just one parameter within collections compared to their ES counterparts which have several optional parameters like index.

ry pushed a commit that referenced this pull request Jul 30, 2021
ry pushed a commit that referenced this pull request Jul 30, 2021
@LionC LionC mentioned this pull request Aug 27, 2021
44 tasks
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.

[Feature Request] Add findLastIndex function to std/collections
3 participants