Make isPrefixOf non-symmetrical

[knuton]

Because map2 discards extra elements in either of its list parameters, the current implementation functions as "either is prefix of the other", not as "first is prefix of second".

Example:

isPrefixOf [1, 2] [1, 2, 3] == isPrefixOf [1, 2, 3] [1, 2]

This can be fixed by comparing list lengths first.

Caveat: I think fixing this behaviour would constitute a breaking change, as implementations may rely on the symmetry.

[mgold]

Regarding a breaking change, elm-package won't let you bump the version to be more severe than the types say it should. I think it's unlikely that anyone will be affected, and this seems like a bug, so a patch change is fine.

Things beyond the scope of this PR:

The example above would make a great unit test, and thereby an opportunity to start a test suite for this library.

It seems that explicit recursion would avoid needed to equate the entire list:

isPrefixOf : List a -> List a -> Bool
isPrefixOf prefix aList =
  case (prefix, aList) of
    (x::xs, y::ys) -> x == y && isPrefix of xs ys
    (x::xs, []) -> False
    ([], _) -> True

[knuton]

Versioning

Regarding a breaking change, elm-package won't let you bump the version to be more severe than the types say it should.

Oh, I wasn't aware of that. I think in cases like this that's unfortunate: The symmetry/non-symmetry is an important aspect of the function's behaviour, it's just not captured by the type.

I think it's unlikely that anyone will be affected, and this seems like a bug, so a patch change is fine.

At least it's a relatively obvious bug, so I doubt that this function is in widespread use. In this sense, it should be reasonable safe.

Implementation

I agree that explicit recursion would be worthwhile here. In fact, as this is library code and even used as a building block in further library code, I would go even further with a slight optimization:

isPrefixOf : List a -> List a -> Bool
isPrefixOf prefix list =
    List.length prefix <= List.length list && isPrefixOfRec prefix list

isPrefixOfRec : List a -> List a -> Bool
isPrefixOfRec prefix aList =
  case (prefix, aList) of
    (x::xs, y::ys) -> x == y && isPrefixOfRec xs ys
    (x::xs, []) -> False
    ([], _) -> True

Testing

Unit tests: Definitely! Shall I just update this branch with both an optimized implementation and an elm-test suite?

[mgold]

I agree that one should be able to release a more breaking change than the types call for, but I also think this is still just a bug.

List.length is actually O(n): it's implemented in terms of foldl. So I think your optimization is actually a... pessimization? You can benchmark it if you feel the need.

As much as I love PRs that come with tests, I think it's best to set them up in another PR.

[knuton]

List.length is actually O(n): it's implemented in terms of foldl. So I think your optimization is actually a... pessimization? You can benchmark it if you feel the need.

Oh, silly me. I like "pessimization". So yes, it only makes sense when comparing deeply nested structures, and those who do that may want to add their own optimization.

Added a test suite in #25 and updated this branch with your proposed implementation plus tests.

[mgold]

Looks like this is the one to merge since it has all the commits.

[mgold]

Looks like this merged all of them. Sweet!

[knuton]

Looks like this merged all of them. Sweet!

Ah, didn't know about that either, that's pretty cool. Would be nice if it created merge commits for each of the PRs instead of just this one, in case one would want to revert partially. Maybe that would be too automagic, though.

Thanks for the prompt merge!