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.head, S.last, S.tail, and S.init #610

Conversation

syves
Copy link
Contributor

@syves syves commented Mar 17, 2019

Closes #570

Initial attempt to generalize head, tail, last and init functions for Foldables.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/init.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/last.js Outdated Show resolved Hide resolved
Copy link
Member

@davidchambers davidchambers left a comment

Choose a reason for hiding this comment

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

Good progress, @syves! I will wait for you to address the outstanding issues before I comment on your changes.

@syves
Copy link
Contributor Author

syves commented Mar 19, 2019

I've addressed the above changes, but for some reason doctest is not running.

20 error code ELIFECYCLE
21 error errno 1
22 error [email protected] doctest: sanctuary-doctest
22 error Exit status 1
23 error Failed at the [email protected] doctest script.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated
function(maybe, x) {
return maybe.isNothing ?
Just (Z.empty (foldable.constructor)) :
Z.map (append (x), maybe);
Copy link
Member

Choose a reason for hiding this comment

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

Something doesn't feel right about using Z.map in conjunction with the isNothing check. If we're to keep the isNothing check we could safely use maybe.value in the Just case:

return maybe.isNothing ?
       Just (Pair (Z.empty (foldable.constructor)) (x)) :
       Just (Pair (Z.append (maybe.value.snd, maybe.value.fst)) (x));

This could be improved by removing the common code from the branches:

return Just (maybe.isNothing ?
             Pair (Z.empty (foldable.constructor)) (x) :
             Pair (Z.append (maybe.value.snd, maybe.value.fst)) (x));
return Just (Pair (maybe.isNothing ?
                   Z.empty (foldable.constructor) :
                   Z.append (maybe.value.snd, maybe.value.fst))
                  (x));

We could even remove the explicit conditional logic by using Z.reduce:

return Just (Pair (Z.reduce (function(_, pair) { return Z.append (pair.snd, pair.fst); },
                             Z.empty (foldable.constructor),
                             maybe))
                  (x));

The problem with this approach is that Z.empty (foldable.constructor) would be evaluated once for each element of the structure. Can you think of a solution to this problem?

Copy link
Member

Choose a reason for hiding this comment

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

The comment above applies to init rather than to tail. I'm sorry for causing confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stored the empty in a var.

index.js Outdated Show resolved Hide resolved
@davidchambers
Copy link
Member

Let's complete this pull request together. I will push some commits to your branch. :)

@davidchambers davidchambers dismissed their stale review March 26, 2019 10:38

Issues have been addressed. :)

@syves syves force-pushed the feature-generalize-head-tail-foldables branch from 6006db3 to 0844962 Compare March 26, 2019 17:13
@syves syves changed the title Feature generalize head tail Foldables array: generalize S.head, S.last, S.tail, and S.init Mar 26, 2019
@syves syves force-pushed the feature-generalize-head-tail-foldables branch from 0844962 to 8588595 Compare March 26, 2019 17:19
@davidchambers
Copy link
Member

I just added fast paths for arrays, so these functions will continue to operate efficiently on arrays. @syves, if you're happy with these changes please squash the commits in preparation for merging. :)

@syves syves force-pushed the feature-generalize-head-tail-foldables branch from 129ad7a to e6485da Compare March 29, 2019 13:10
@davidchambers
Copy link
Member

Thank you, @syves!

@davidchambers davidchambers merged commit ecde7d4 into sanctuary-js:master Mar 31, 2019
@syves syves deleted the feature-generalize-head-tail-foldables branch May 6, 2019 13:33
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