Deduplicate testing code - with records #200
Conversation
Migrating common tests on the todo list
Tests todo
Applied some other renaming todos
| @@ -0,0 +1,1099 @@ | |||
| module Test.AllTests where | |||
There was a problem hiding this comment.
All tests are merged into this file. It replaces all the files in test/Test/Data. Those can be deleted after this PR goes though.
| @@ -0,0 +1,197 @@ | |||
| module Test.API where | |||
There was a problem hiding this comment.
This file outlines the API for the list varieties
| @@ -0,0 +1,105 @@ | |||
| module Test.UpdatedTests(updatedTests) where | |||
There was a problem hiding this comment.
This is where we call the common tests for each list variety
| @@ -5,6 +5,9 @@ Notable changes to this project are documented in this file. The format is based | |||
| ## [Unreleased] | |||
|
|
|||
| Breaking changes: | |||
| - Rename `scanrLazy` to `scanlLazy` and fix parameter ordering (#161) | |||
| - Rename `group'` to `groupAll` (#182) | |||
| - Change `Alt ZipList` to satisfy distributivity (#150) | |||
|
|
|||
There was a problem hiding this comment.
Is there a way to implement this PR without the above breaking changes? The changes could still be made in a separate PR that we could wait to merge until another breaking PS release is made.
There was a problem hiding this comment.
These changes have already been made and released; I think this is just a bad merge.
There was a problem hiding this comment.
I'm planning on merging this after #191 goes though, so I incorporated changes from that PR here too.
But yes, this is a pretty sloppy PR. Just wanted to upload the WIP to share some context during yesterday's meetup.
| -- replicate num x = go num Nil | ||
| -- where | ||
| -- go n xs | n < 1 = xs | ||
| -- | otherwise = go (n - 1) (x : xs) |
There was a problem hiding this comment.
I'm wondering whether this should be added. Does anyone use this? Would anyone use this?
There was a problem hiding this comment.
I'm wondering the same. Current plan is to just provide a specialized replicate for List.Lazy, and otherwise use Unfoldable's replicate here. So I'd remove this commented block during clean-up.
The full API proposal can be found in #183 (comment). Some related questions about what to include or not can be found at the end of that comment.
src/Data/List/NonEmpty.purs
Outdated
| -- | Create a list containing a range of integers, including both endpoints. | ||
| -- Todo, rewrite this without unsafe workaround (if necessary) | ||
| range :: Int -> Int -> NonEmptyList Int | ||
| range start end = unsafePartial fromJust $ fromList $ L.range start end |
There was a problem hiding this comment.
Perhaps this should be rewritten so that the start arg is always included in the range and guarantees the NonEmpty part of the returned value, so we don't need to wrap it in a Maybe?
There was a problem hiding this comment.
Which implementation do you like more?
range :: Int -> Int -> NonEmptyList Int
range start end =
cons' start (fromMaybe L.Nil (L.tail (L.range start end)))range :: Int -> Int -> NonEmptyList Int
range start end | start < end = cons' start (L.range (start + 1) end)
| start > end = cons' start (L.range (start - 1) end)
| otherwise = singleton startWe could also write one that doesn't rely on basic List's range at all.
There was a problem hiding this comment.
I think the second one looks good.
| } | ||
|
|
||
| type CommonDiffEmptiability c cInverse canEmpty nonEmpty cPattern = | ||
| { makeCollection :: forall f a. Foldable f => f a -> c a |
There was a problem hiding this comment.
To reduce verbosity and help show similarities (though I don't think this would help readability), should items like makeCollection be stored as a Row that gets added into each differing item via row polymorphism?
There was a problem hiding this comment.
That could be a nice adjustment, but not sure the deduplication would be enough to justify the readability tradeoff and Row + Record overhead. I might be misimagining what changes are involved with this proposal though, so would be interested in seeing it implemented.
Another option could be to do something like this:
https://github.com/milesfrain/purescript-lists/pull/4/files
There was a problem hiding this comment.
Yeah... I'm not sure it's worth it either due to the readability tradeoff.
A version of #199 using records instead of typeclasses. Addresses proposal in #183.
WIP - A bit messy at the moment. Will make another PR with cleaner commit history, but dropping this here in the meantime for easier visibility of proposed changes.