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

derive ‘size’ from ‘reduce’ #61

Merged
merged 1 commit into from
Jun 16, 2017
Merged

Conversation

davidchambers
Copy link
Member

See sanctuary-js/sanctuary#414 for prior discussion.

Copy link
Member

@gabejohnson gabejohnson left a comment

Choose a reason for hiding this comment

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

LGTM

@safareli
Copy link
Member

@davidchambers Tuple es well as These might return unpredictable result. technically you can define size for betraversables too :d (but we don't have bitraversable in FL)

eq(Z.size(Identity('quux')), 1);
eq(Z.size(Nothing), 0);
eq(Z.size(Just(0)), 1);
eq(Z.size(Tuple('abc', 123)), 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

Tuple […] might return unpredictable result.

I agree that this is surprising initially, but it seems correct. Haskell agrees:

> length ("abc", 123)
1

What do you suggest, @safareli?

Copy link
Member

Choose a reason for hiding this comment

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

It's not unpredictable if the user is familiar with how reduce works for their data type. Foldable is in the signature after all.

Copy link
Member

Choose a reason for hiding this comment

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

reduce works for their data type

Exactly!

@davidchambers davidchambers merged commit e98b2e0 into master Jun 16, 2017
@davidchambers davidchambers deleted the davidchambers/size branch June 16, 2017 13:30
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.

3 participants