Skip to content
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

Generic instance #5

Closed
akegalj opened this issue Sep 20, 2016 · 6 comments
Closed

Generic instance #5

akegalj opened this issue Sep 20, 2016 · 6 comments

Comments

@akegalj
Copy link

akegalj commented Sep 20, 2016

Is there a reason of not heaving default derived Generic instance ?

@rgrempel
Copy link
Owner

Generally speaking, having a default Generic instance is equivalent to exporting your constructors, so you might not want one for the same reasons you might not want to export your constructors.

For instance, in this case, the Int53 constructor takes a Number internally. However, we don't export it, because not all numbers are integers (we just use Number as a convenience to get all 53 bits). Since we want to limit Int53 values to actual integers, we can't export the constructor.

If we had a default Generic instance, you could do something like this:

badInt53 = fromSpine (SProd "Data.Int53.Int53" [\_ -> SNumber 27.3]) :: Maybe Int53
-- Just (Int53 27.3)

... which we wouldn't want to be possible.

Now, it's possible that we could write a Generic instance manually that would be fine ... it would probably use fromNumber at some point to take an SNumber and turn it into a Maybe Int53. But I haven't entirely thought that through.

@akegalj
Copy link
Author

akegalj commented Sep 20, 2016

However, we don't export it, because not all numbers are integers
I agree, didn't think about it enough.

Yes, we are using custom Generic instance, but I guess I was slightly annoyed with writing additional newtype wrapper so that I can define Generic. But you are right, adding generic would be wrong in this case. Thank you for your explanation.

Note that you can derive Eq and Ord . I think their derived instance is the same as your manual instances.

@akegalj akegalj closed this as completed Sep 20, 2016
@rgrempel
Copy link
Owner

Actually, I was thinking of adding a manual Generic instance to Int53 itself, so you wouldn't need the newtype. Something like this:

ea72550

Does that look like it might be helpful? I can merge it if so.

@rgrempel rgrempel reopened this Sep 20, 2016
@akegalj
Copy link
Author

akegalj commented Sep 21, 2016

We are using more simple instance of Generic (it might be wrong general case but it suits our needs). I don't have time now so will review it tomorrow (and read more about Generic, don't have much experience with it).

@rgrempel
Copy link
Owner

I've published version 2.1.0, with the manually-written Generic instance.

@akegalj
Copy link
Author

akegalj commented Oct 1, 2016

Thank you. Sorry for not being able to review your code (I had it on my todo but can't allocate free time currently). Many thanks <3

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

No branches or pull requests

2 participants