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(toFilled): add toFilled #154

Merged
merged 11 commits into from
Jul 11, 2024
Merged

feat(toFilled): add toFilled #154

merged 11 commits into from
Jul 11, 2024

Conversation

wondonghwi
Copy link
Contributor

close: #118

The fill function was added to es-toolkit, and since fill mutates the original array, a new function that does not modify the original array was needed. To address this, we added a new function, toFilled.

I have added a function called toFilled that does not modify the original array. This function fills a specified portion of an array with a given value but returns a new array instead of modifying the original one. Additionally, this function allows for the use of negative indices, meaning you can specify positions from the end of the array backward. The toFilled function works similarly to the fill function but stands out by not altering the original array.

In the benchmark results, since lodash does not have a toFilled function, I compared it with lodash’s fill function. The comparison showed that the toFilled function performed better in terms of performance.

The implementation of this function was inspired by this issue.
스크린샷 2024-07-10 오후 11 36 13

Copy link

vercel bot commented Jul 10, 2024

@wondonghwi is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d48900f) to head (0899f52).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #154   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        74    +1     
  Lines          409       417    +8     
  Branches        54        57    +3     
=========================================
+ Hits           409       417    +8     

raon0211 and others added 3 commits July 11, 2024 08:55
@raon0211
Copy link
Collaborator

Thanks for your great contribution!

I wanted to discuss the behavior of toFilled in comparison to the original fill function.

One key difference I've noticed is that fill supports negative indices, whereas toFilled does not. Since lodash's fill function does not support negative indices, I believe toFilled should follow and not support them either.

What are your thoughts on this?

@wondonghwi
Copy link
Contributor Author

Hello @raon0211,

Based on my observations and tests, it appears that Lodash's fill function does indeed support negative indices. Given this, it seems appropriate that toFilled, as well as esToolkit's fill function, should also support negative indices to maintain consistency. Here is an example that demonstrates the behavior:

image

Given this behavior, I believe it would be beneficial to enhance both toFilled and esToolkit's fill functions to support negative indices. I would be interested in working on this enhancement if it is deemed necessary. What are your thoughts on this?

Copy link
Collaborator

@raon0211 raon0211 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@raon0211
Copy link
Collaborator

Given this behavior, I believe it would be beneficial to enhance both toFilled and esToolkit's fill functions to support negative indices.

Got this. Could you fix the behavior for fill, too?

@raon0211 raon0211 merged commit ea66835 into toss:main Jul 11, 2024
5 of 7 checks passed
@wondonghwi
Copy link
Contributor Author

@raon0211

Sure :)

I will raise this issue and proceed to modify the fill function accordingly!

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.

Support for toFilled
4 participants