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(take): add typing #7

Merged
merged 6 commits into from
Oct 9, 2023
Merged

Conversation

StyleShit
Copy link
Contributor

@StyleShit StyleShit commented Oct 3, 2023

It's basically for fun. I've also changed the implementation a little bit, WDYT?
it probably breaks BC, I did it only for the sake of typing it 😅

@bengry
Copy link
Owner

bengry commented Oct 5, 2023

It's basically for fun. I've also changed the implementation a little bit, WDYT? it probably breaks BC, I did it only for the sake of typing it 😅

Thanks for the contribution!

The types are looking good from a quick glance, but the implementation should remain the same.
i.e. take(array, count) should return [] when count <= 0.
I don't believe that taking from the end is the goal of take.

I'd be happy to accept a takeLast function implementation that does this though, that's the "mirror" of take, if you'd like.

@StyleShit
Copy link
Contributor Author

StyleShit commented Oct 6, 2023

yeah, sure, I've just reverted the logic changes.
BTW, I think you should run the CI workflows also on PRs instead of only on push, but that's for another PR.
I can open another PR for takeLast() / takeRight(), but let's first finish with this one :)

Here is a tsplay so you can play with it


You should thank (or blame?) Omri Mor for the existence of this PR, he sent me this repo while I was bored 😆

@bengry
Copy link
Owner

bengry commented Oct 6, 2023

yeah, sure, I've just reverted the logic changes

Thanks

BTW, I think you should run the CI workflows also on PRs instead of only on push, but that's for another PR.

For sure, I'll make that change.

Here is a tsplay so you can play with it

Seems like there's a bug in the type's implementation - see d here.

You should thank (or blame?) Omri Mor for the existence of this PR, he sent me this repo while I was bored 😆

Will do 😂

Copy link
Owner

@bengry bengry left a comment

Choose a reason for hiding this comment

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

Once the outstanding comments are fixed, I think we're good to merge this :)

src/functions/take/take.spec.ts Outdated Show resolved Hide resolved
src/functions/take/take.ts Show resolved Hide resolved
Copy link
Owner

@bengry bengry left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to also open a PR for a takeRight function if you want to.

@bengry bengry merged commit db4a64c into bengry:main Oct 9, 2023
1 check passed
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.

2 participants