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

array: generalize S.takeLast and S.dropLast #630

Merged
merged 1 commit into from
Apr 27, 2019

Conversation

davidchambers
Copy link
Member

The implementations are not as efficient as they could be, but I want to avoid spreading complexity currently confined to _takeDrop.

Copy link
Member

@Bradcomp Bradcomp left a comment

Choose a reason for hiding this comment

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

I like this because it's simple and straightforward. It looks good to me.

One thing to note is that by generalizing these functions we are creating two partial functions. There is an implicit type requirement that the Foldable is also a finite structure. If we give it an infinite structure the program won't halt, as you can't reverse an infinite structure.

This concern is mitigated for me due to Z.reverse already existing, and because if you do pass a potentially infinite structure to one of these you're probably already doing something wrong. It is more of a theoretical concern.

@masaeedu
Copy link
Member

If I'm right about the applicative constraint not being required in #627, then we probably don't need it here as well.

@davidchambers
Copy link
Member Author

You have raised a good point, @Bradcomp; I had not considered infinite structures. I would not expect these functions to work on infinite structures, so I do not find the behaviour surprising, but it is a shame that the type signatures do not accurately convey the constraints.

#627 (comment) applies here too, @masaeedu, so I will merge this pull request in its current form. If it turns out that I have misunderstood the constraints, we can relax the constraints on S.take, S.drop, S.takeLast, and S.dropLast in another pull request. :)

@davidchambers davidchambers merged commit 9bace6f into master Apr 27, 2019
@davidchambers davidchambers deleted the davidchambers/take-drop-last branch April 27, 2019 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants