Skip to content

UInt instances? #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jamesdbrock opened this issue Sep 18, 2021 · 8 comments
Open

UInt instances? #17

jamesdbrock opened this issue Sep 18, 2021 · 8 comments

Comments

@jamesdbrock
Copy link
Member

Should we add some of these instances?

https://pursuit.purescript.org/packages/purescript-uint-instances/

@maxdeviant
Copy link
Member

Looks like there was some previous discussion in #7 on this.

From what I can tell, the outcome of that issue was the addition of an Arbitrary instance in d3a6424.

The Arbitrary instance was then removed in 6820b50 when the QuickCheck dependencies were moved into the test dependencies.

Looking at the purescript-uint-instances package, it adds the following instances that we do not currently have:

@chtenb
Copy link
Member

chtenb commented Jun 27, 2022

I came here because I was looking for an Arbitrary instance for UInt. Is there a reason why we would not want to expose the Arbitrary instance in the API?

@garyb
Copy link
Member

garyb commented Jun 27, 2022

I think providing something in terms of MonadGen would be preferable since that has a much lower dependency overhead.

@jamesdbrock
Copy link
Member Author

jamesdbrock commented Jun 28, 2022

I would love a MonadGen PR @chtenb ... or Arbitrary, or whatever you think is best

@chtenb
Copy link
Member

chtenb commented Jun 28, 2022

Right, so if MonadGen is preferred then we would have something like

chooseUInt :: forall gen. MonadGen gen => UInt -> UInt -> gen UInt
chooseUInt a b = fromInt $ chooseInt (toInt a) (toInt b)

but we're using the chooseInt from quickCheck here, so that doesn't solve the dependency overhead.

Wouldn't it perhaps be better to add this to the quickCheck library instead then? The quickcheck lib would gain uint as a dependency, but that's a very small library. Another advantage would be that we could add Arbitrary while we're at it.

@jamesdbrock
Copy link
Member Author

You don't need the quickcheck dependency, you can just use the chooseInt provided by the MonadGen. See for example https://pursuit.purescript.org/packages/purescript-float32/2.0.0/docs/Data.Float32.Gen

@chtenb
Copy link
Member

chtenb commented Jun 30, 2022

You're right @jamesdbrock
So in that case I'd say we add the chooseUInt based on MonadGen to this package and then also provide the arbitrary instance in the quickcheck package, if the folks there are okay with that.

@chtenb
Copy link
Member

chtenb commented Jul 2, 2022

We seem to have overlooked that genUInt already exists in UInt.Gen 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants