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 58 forks source link

Add allSame #120

Closed jonathanfishbein1 closed 4 years ago

pzp1997 commented 5 years ago
  1. I'm not quite convinced that this function is useful enough for inclusion in list-extra. Could you please provide a real-world use case as per the contribution guidelines?

  2. I find the way your function behaves on the empty list surprising. I would expect the function to return True but your implementation returns False. My expectation is based off of the idea that if there are no elements in the list then it holds vacuously that all the elements are the same. (This is the same reason why List.all [] == True.)

  3. The use of comparable here is unnecessary since you only need equality to check that all the elements are the same. For example, the following implementation does the same thing (except for how it behaves on the empty list, see above point) without the comparable constraint.

allSame : List a -> Bool
allSame list =
    case list of
        [] ->
            True

        x :: xs ->
            List.all (\y -> x == y) xs
  1. Good job adding tests. I think a few more would be necessary though if this function were to be included in list-extra: empty list, all same except for the head, all same except for something in the tail, etc.
jonathanfishbein1 commented 5 years ago

Hmm yea, I didn't consider the edge cases.

I'm using it's implementation in a linear algebra package I'm currently writing. The function would verify that the lengths of all the vectors passed into the constructor of a matrix are the same length. Below is the current implementation.

makeMatrix : List (Vector (List a)) -> Result String (Matrix (List (Vector (List a))))
makeMatrix listOfVectors =
    if allSameBy (\(Vector x) -> List.length x) listOfVectors then
        Ok <|
            Matrix listOfVectors

    else
        Err "list has differnt inner list length: Malformed input"

If you think that there is a use case for allSame in list-extra I'd be happy to modify the implementation and add additional tests

Chadtech commented 5 years ago

Thanks @jonathanfishbein1

Im also a little unsure about this function.

  1. On whether it should return True or False for empty lists, yeah its unclear. Seems very contextual, with True making sense in some use cases and False in others, so its hard to make a general function for everyone.

  2. Thanks for the use case. I'm kind of torn about it. Your use case is basically validation, and I think validation functions usually work well with types like a -> Maybe b, where a b cannot possibly be invalid, and they usually dont work so well with types like a -> Bool, because when the function gives True, it is valid, but it passes along data which by definition can be invalid. Its kind of like how a new car loses a tremendous amount of its value as soon as it leaves the lot.

But that said, I really dont know how to make a good List (List a) that by definition has all of its List a of the same length. So maybe just checking the Bool is as good as it gets.

We can keep this PR open and keep thinking about it. I'll try and picture some use cases for this function too.

jonathanfishbein1 commented 5 years ago

Thank you @pzp1997 and @Chadtech for the guidance.

I've determined I don't really like the matrix constructor pattern. It's a weakness of haskell and elm that you cant define that a Matrix should be a list of Vectors of equal length as opposed to a jagged array type. From what I know about Idris and dependently typed languages you can constrain the type in that manner. At this point I don't think I want to add this runtime validation that would be better suited as a compiler error in a dependently typed language.