Closed m-bock closed 4 years ago
Seems like a good idea to me
It will also need to be published to pursuit. (Mentioning this because the current latest tag in the repo is not uploaded to pursuit. The latest version there is 3.1.0)
Sorry everyone, I've been a bit preoccupied these last few days. However, I very much appreciate the additions provided. As far as the next version is concerned, we'll have to debate a little bit on which features should be considered fundamental to the package.
I definitely think the Apply instance fix is essential, but I'm still considering the other PRs - which ones are genuinely necessary for the continued function of this package? I'm a little tied up right now, so I can't investigate on my own, but I would really appreciate any comparisons you can provide me. Tonight I'll be going through each file change for the PRs and making my decision.
\cc @3ddyy @thought2
For the fix PR, i'd say that one's pretty necesscary for overall consistency and will make the package more pleasant to work with in the future. It also adds the CommutativeRing instance, which i don't see any point in not having.
I left a comment on the Semigroup/Monoid PR explaining why i think that is the correct/canonical instance for Vec
.
When it comes to the Bind/Monad PR, i again don't see why we shouldn't have that instance, as it upholds the "zippy" nature of the Apply
and Semigroup
instances. Vec
is already quite similar to ZipList
, aside from the fact that it can't have a Bind
instance due to the length of the lists not being known from the type. In many ways, Vec
is a better version of ZipList
, which is more robust and can do more due to the length being known from the type. One of the things it can do that ZipList
can't, is to have a Bind
instance. The Bind
instance is even zippy, in the sense that the element at index 0 can not have any effect on the element at index 1 through the use of bind
. Essentially, what you would be doing using do
notation, would be working with each element of the Vec
individually and independently, and it would be useful if you want to do the same thing to many sets of elements. Without this, you'd probably use ZipList
and ado
notation for that, but it's more limited, for example in that the contents of one List can't depend on those of another. That's why i think a Monad
instance here could be quite useful. (I could try coming up with an example, if you're not fully convinced)
I've added a new commit - https://github.com/bodil/purescript-sized-vectors/commit/098eb06527be3cdb06474d3a2ae7ed84f538cf30
This uses purescript-quickcheck-laws and adds an Arbitrary instance for Vec
. What do you think? I think it's well set to be published as 5.0.0
\cc @3ddyy
Sounds good 👍
Very nice to have an arbitrary instance. When using the package in practice this was one of the reasons I often had to add a newtype wrapper around Vec
. Another one was for DecodeJson
and EncodeJson
from argonaut.
Would it maybe make sense to add this, too? If you agree I can go ahead and make a PR for this.
Thanks for the improved Show
instance. Of course this is better than vecN ..
as this exists only for a few n
's.
Regarding your question "which features should be considered fundamental to the package". I'd definitely say that the apply instance is a clear bug and the rest are more features.
With the exception of a small thing, it looks like we're ready for this now? cc @athanclark https://github.com/bodil/purescript-sized-vectors/commit/6e2ae48925778f436cebc754e7e395a59169ab19#r37033265
And she's published under v5.0.0! Thank you for all your help
If the outstanding PR's get merged, I'd suggest that it's time to release a new version of the library.
As the new correct apply instance is a breaking change, I'd say this would be.
5.0.0
then?