NonEmpty arrays #127
NonEmpty arrays #127
Conversation
|
reminder to myself to add an explicit export list |
| show (NonEmptyArray xs) = "(NonEmptyArray " <> show xs <> ")" | ||
|
|
||
| derive newtype instance eqNonEmptyArray :: Eq a => Eq (NonEmptyArray a) | ||
| derive newtype instance eq1NonEmptyArray :: Eq1 NonEmptyArray |
There was a problem hiding this comment.
Presumably this won't work with 0.11.7? I wonder if it might be nice to make this code work with 0.11.7 first so that we can release it as a minor level change before we release 0.12, and then derive these instances later?
There was a problem hiding this comment.
I've been compiling with 0.11.7, and it isn't complaining. Will this bug out at runtime?
There was a problem hiding this comment.
Given
data X a = X
derive instance eq1X :: Eq1 Xtry.purescript.org gives me "Cannot derive a type class instance for Data.Eq.Eq1 X since instances of this type class are not derivable." Is it possible you're using master by accident?
There was a problem hiding this comment.
Nope. 0.11.7 as installed by Homebrew. Did a clean install, deleted output and bower_components and it's all compiling fine for me.
I don't know what to say! Is there a commit after 0.11.7 that would have changed this behavior? Or is it possible that the deployed version on Try PureScript is wonky?
There was a problem hiding this comment.
This was only merged very recently: purescript/purescript#3207
What is the output of your purs --version? Is it possible you've got a newer purs installed locally to that project which is overriding the global one? Also can you link me to the homebrew formula you're using? I've checked locally with my 0.11.7 and I get the same thing; "instances of this type class are not derivable".
There was a problem hiding this comment.
@hdgarrood this is a derive newtype instance, so I think it's fine.
There was a problem hiding this comment.
lmao of course, I'm being silly. thanks
src/Data/Array/NonEmpty.purs
Outdated
| derive newtype instance altNonEmptyArray :: Alt NonEmptyArray | ||
|
|
||
| -- | Internal - adapt an Array transform to NonEmptyArray | ||
| adapt :: forall a b. (Array a -> Array b) -> NonEmptyArray a -> NonEmptyArray b |
There was a problem hiding this comment.
I realise this is internal, but even so I'm a little uneasy about defining this function with this type, as it's actually partial; if the function you provide can return an empty array you're in trouble. I think it might be better to use unsafeCoerce in individual implementations within this module; it's then easier (imo) to verify what's going on and that the coercion is justifiable.
There was a problem hiding this comment.
Good point. Thanks.
There was a problem hiding this comment.
For the moment, I'm going to make a small concession on this and just rename these to unsafeAdapt. I can always go back and do the unsafeCoerce option later. That said, I do think that unsafeAdapt, along with docs, should tell anyone hacking on the library what to look for / be aware of.
|
@hdgarrood thanks for the input. It is indeed very much WIP, but I'm happy to fix things as I'm working. |
matthewleon
force-pushed
the
nonempty
branch
2 times, most recently
from
0db2408 to
830a15b
Compare
|
Is this good for "final" review now? 🙂 |
|
Sadly I'm not confident yet that it is. I'll give it a look over the next couple of days and try to fix up any key, missing things. |
|
No worries, I just wondered if it was waiting on one of us to approve now or if it was something you still considered WIP 🙂 |
Yes, WIP for now, I'm just taking way too long. Time to wrap it up. |
|
Look who you're talking to 😄 no pressure meant at all. |
matthewleon
force-pushed
the
nonempty
branch
3 times, most recently
from
3008e76 to
d1366cb
Compare
Functions on NonEmpty arrays, lacking docs and tests, and potentially some instances.
they can wait until Data.Array group functions use NonEmptyArray
|
This is ready for review. |
|
Looks like the build is failing due to JS formatting pickyness 😉... I'll review the changes proper now though. |
|
I’ll fix this shortly.
…On Sat, 10 Mar 2018 at 12:29, Gary Burgess ***@***.***> wrote:
Looks like the build is failing due to JS formatting pickyness 😉... I'll
review the changes proper now though.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARBSzZuivAvi5B-F2qCuF0l9fIVbWIFks5tdA2OgaJpZM4RmIls>
.
|
src/Data/Array/NonEmpty.purs
Outdated
| -- Note that this is unsafe: if the transform returns an empty array, this can | ||
| -- explode at runtime. | ||
| unsafeAdapt'' :: forall a b c d. (a -> b -> Array c -> Array d) -> a -> b -> NonEmptyArray c -> NonEmptyArray d | ||
| unsafeAdapt'' f = unsafeAdapt' <<< f |
There was a problem hiding this comment.
I'd probably drop the prime'd varieties of this, we can just compose with unsafeAdapt where it's used, or unsafeAdapt <<< map f for the cases that need the double-prime'd version (map here working out as compose again, via the Functor instance for functions).
There was a problem hiding this comment.
Same goes for the adaptAny variants.
src/Data/Array/NonEmpty.purs
Outdated
| singleton = NonEmptyArray <<< A.singleton | ||
|
|
||
| range :: Int -> Int -> Maybe (NonEmptyArray Int) | ||
| range x y = fromArray $ A.range x y |
There was a problem hiding this comment.
Maybe like with replicate this should always produce at least 1 value? Actually, isn't that basically guaranteed anyway? Unless "reverse-ranges" don't work, but I don't see why they couldn't (although obviously that'd be more of a change as we'd want it to behave consistently with normal arrays).
kill `unsafeAdapt'`, `unsafeAdapt''`, `adaptAny'`, `adaptAny''`
|
@garyb thanks for the review. This should be ready for another round now. |
work in progress
addresses #120