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/unstable): support Iterable argument in takeWhile() #5911

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Liam-Tait
Copy link
Contributor

Support Iterable for collections takeWhile

#5470

@Liam-Tait Liam-Tait marked this pull request as ready for review September 4, 2024 16:03
@Liam-Tait Liam-Tait requested a review from kt3k as a code owner September 4, 2024 16:03
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.27%. Comparing base (6a4eb6c) to head (b36d8ec).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5911      +/-   ##
==========================================
- Coverage   96.28%   96.27%   -0.01%     
==========================================
  Files         496      497       +1     
  Lines       39605    39621      +16     
  Branches     5841     5843       +2     
==========================================
+ Hits        38134    38147      +13     
- Misses       1429     1432       +3     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Liam-Tait Liam-Tait changed the title refactor(collections): replace array argument with iterable` for take… refactor(collections): takeWhile replace array argument with Iterable Sep 5, 2024
@iuioiua iuioiua changed the title refactor(collections): takeWhile replace array argument with Iterable feat(collections): support Iterable input in takeWhile() Sep 5, 2024
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Nice! LGTM. @kt3k, how do you prefer we handle this, in terms of stability?

@kt3k
Copy link
Member

kt3k commented Sep 5, 2024

how do you prefer we handle this, in terms of stability?

I'd prefer to avoid overloading with this.

How about considering this as 'refactor' or 'fix'?

@kt3k
Copy link
Member

kt3k commented Sep 5, 2024

Or, maybe we can just land this as feat(collections/unstable) without overloading, but with note of something like Iterable input is unstable. WDYT?

@iuioiua
Copy link
Collaborator

iuioiua commented Sep 5, 2024

Or, maybe we can just land this as feat(collections/unstable) without overloading, but with note of something like Iterable input is unstable. WDYT?

SGTM

@iuioiua
Copy link
Collaborator

iuioiua commented Sep 9, 2024

Sorry for the delay. We're deciding how to best handle these changes that should probably be considered unstable.

@Liam-Tait
Copy link
Contributor Author

No problem. Let me know if you need anything from me

@Liam-Tait Liam-Tait changed the title feat(collections): support Iterable input in takeWhile() feat(collections): support Iterable argument in takeWhile() Sep 9, 2024
@kt3k
Copy link
Member

kt3k commented Sep 19, 2024

We decided to separate unstable feature from stable features more clearly (as described in #5920 ) to make it clear which part is stable and which part is not.

We are going to move this PR's implementation to unstable_take_while.ts (which will be available from JSR with the path jsr:@std/collections/unstable-take-while ) before merging, and keep the existing implementation as is.

@kt3k kt3k changed the title feat(collections): support Iterable argument in takeWhile() feat(collections/unstable): support Iterable argument in takeWhile() Sep 19, 2024
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. We've made a few tweaks, and now, LGTM! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants