elm-community / list-extra

Convenience functions for working with List.
http://package.elm-lang.org/packages/elm-community/list-extra/latest
MIT License
135 stars 59 forks source link

Add `remove`, clean up filter/remove/drop functions #6

Closed shmookey closed 8 years ago

shmookey commented 8 years ago

remove x xs removes the first occurrence of x from xs (same as Haskell's delete)

This PR also implements the filter/remove/drop cleanup discussed in #7, such that:

jvoigtlaender commented 8 years ago

Surely the delete function can be implemented more efficiently. You say the function is from Haskell. What's the source? How is it implemented in Haskell?

jvoigtlaender commented 8 years ago

I'm also not happy with the naming. Seems arbitrary. How does a user know that "delete" means only one, but "remove" means all?

To decide on naming, probably first have to resolve https://github.com/elm-community/list-extra/issues/7.

jvoigtlaender commented 8 years ago

Also, though that is really the maintainer's decision: I don't think pull requests should contain version bumps. The maintainer will bump versions with the help of the automatic semver tools whenever a release is done.

shmookey commented 8 years ago

Thanks for the feedback!

jvoigtlaender commented 8 years ago

Elm does tail recursion optimization. I don't know what you mean by tail recursion modulo cons.

My choice of name for your delete function would probably be deleteFirst. Or removeFirst, whichever of "delete"/"remove" gets consistently used in the library.

shmookey commented 8 years ago

Whoops, yeah, First is definitely the right suffix for this. By "modulo cons" I mean it would be tail recursive but for prepending an element to the returned list.

jvoigtlaender commented 8 years ago

Oh, then the same would apply here. Actually, I don't think GHC would optimize that, nor would it optimize a true tail recursion into a loop. Elm will at least optimize true tail recursion into a loop, but not tail recursion modulo cons.

shmookey commented 8 years ago

Reverted elm-package.json, removed remove, renamed delete to deleteFirst and switched to a recursive implementation

mgold commented 8 years ago

Tail recursion modulo cons is not tail recursion, since the stack frame cannot be reused for the subsequent call, and the memory consumption and call stack grow with the depth of recursion. Unless specially optimized (or perhaps by laziness?), the cons cannot happen until the recursive call returns.

Removing all instances of an element with filter is trivial. Removing the first instance of an element may be useful. This can be generalized into removing the first n elements that satisfy a predicate.

jvoigtlaender commented 8 years ago

@mgold, it's really irrelevant here, but what @shmookey calls tail recursion modulo cons could be optimized into a loop.

mgold commented 8 years ago

Everything looks good to me except the docs for unique are missing, definitely a period, and probably another sentence and an example.

shmookey commented 8 years ago

@mgold good idea, done.

jvoigtlaender commented 8 years ago

@abadi199, as the maintainer of this package, what is your reaction here?

abadi199 commented 8 years ago

I agree with all the changes here. I'll merge this PR so we can also move forward on #5