Match nub* functions with Array #179
Conversation
This reverts commit 02f2b38.
|
If this looks good for I'm also thinking of refactoring the test code and extracting common material, so we don't have as much copy-paste going on between the three files. I assume circular dependencies prevent us from upgrading the test framework to something that prints more info upon failures. I'm a fan of |
|
Yes, everything is very basic in the core libraries for this reason. test-unit is not an option both because of circular dependencies and because there is a policy that core libraries cannot depend on non-core libraries. |
|
This looks great so far 👍 |
|
This looks good to me! |
Co-authored-by: Thomas Honeyman <admin@thomashoneyman.com>
| nubBy eq' (x : xs) = x : nubBy eq' (filter (\y -> not (eq' x y)) xs) | ||
| nubByEq :: forall a. (a -> a -> Boolean) -> List a -> List a | ||
| nubByEq _ Nil = Nil | ||
| nubByEq eq' (x : xs) = x : nubByEq eq' (filter (\y -> not (eq' x y)) xs) |
There was a problem hiding this comment.
This was never stack-safe. Reported in #194
| let { found, result: s' } = insertAndLookupBy p a s | ||
| in if found | ||
| then step (go s' as) | ||
| else Cons a (go s' as) |
There was a problem hiding this comment.
This doesn't look stack safe, but I don't know of a good way to fix it. The stack-safety strategy for strict nubBy involved reversing the final result, but that can't be done here with infinite lazy lists. Reported in #194
There was a problem hiding this comment.
I think it is mostly stack-safe, because it's the caller's responsibility to force the thunks and most of the recursive calls are deferred. However, there could potentially be enough duplicate elements in a row to blow the stack (since we do recursively force thunks in that case).
CHANGELOG.md
Outdated
| Breaking changes: | ||
| - Convert `nub`/`nubBy` to use ordering, rather than equality (#179) | ||
|
|
||
| New features: | ||
| - Add `nubEq`/`nubByEq` (#179) |
There was a problem hiding this comment.
Could have alternatively written this as:
Breaking changes:
- Rename
Eq-basednub/nubBytonubEq/nubByEq(Match nub* functions with Array #179)New features:
- Add
Ord-basednub/nubBy(Match nub* functions with Array #179)
There was a problem hiding this comment.
I like the way you've formulated it in the changelog currently.
| @@ -332,8 +334,15 @@ testListLazy = do | |||
| assert $ nub (l [1, 2, 2, 3, 4, 1]) == l [1, 2, 3, 4] | |||
|
|
|||
| log "nubBy should remove duplicate items from the list using a supplied predicate" | |||
| let nubPred = \x y -> if odd x then false else x == y | |||
| assert $ nubBy nubPred (l [1, 2, 2, 3, 3, 4, 4, 1]) == l [1, 2, 3, 3, 4, 1] | |||
| let nubPred = compare `on` Array.length | |||
There was a problem hiding this comment.
Could you add a test which ensures that nub behaves sensibly on infinite lists please? Perhaps
log "nub should not consume more of the input list than necessary"
assert $ (take 3 $ nub $ cycle $ l [1,2,3]) == l [1,2,3]
There was a problem hiding this comment.
Should this test be applied to all nub* functions, or just nub? I just added the single test.
There was a problem hiding this comment.
I think it’s worth doing it for nub and nubEq but we probably don’t need it for the *By variants, as if it works for nub it’ll probably work for nubBy too.
| - Fixed Lazy List docs where original list is returned instead of Nothing (#169) | ||
| - Migrated to GitHub Actions (#177) | ||
| - Changed `foldM` type signature to more closely match `foldl` (#165) | ||
| - Improved `foldr` performance on large lists (#180) | ||
| - Generated changelog and add PR template (#187) |
There was a problem hiding this comment.
I believe consensus for changelog is to use past tense
|
I believe this PR can be merged, but I think it would be good for @hdgarrood and/or @MonoidMusician to have one more glance over it before we do. |
|
Looks good to me! Did a couple more tests in the repl just to verify |
Fixes #178
Data.ListData.List.LazyData.List.NonEmptyEdit: Now with
nub/nubByfor lazy list too.Old comment:
Since
Data.List.Lazydoesn't havesortyet, I don't think it's in the scope of this PR to add the optimizedOrd-basednub/nubByto that module. It will just be stuck withnubEq/nubByEqfor now.If we do want to add
sorttoData.List.Lazy(in another PR), we might be able to find some inspiration from Haskell's implementation.