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

Add newtype support #14

Merged
merged 5 commits into from
Jul 24, 2019
Merged

Add newtype support #14

merged 5 commits into from
Jul 24, 2019

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented Jul 23, 2019

No description provided.

@treeowl
Copy link
Contributor Author

treeowl commented Jul 23, 2019

@chessai You seemed interested in this too.

@treeowl treeowl force-pushed the newtype branch 2 times, most recently from b2063f6 to 76a1db7 Compare July 23, 2019 02:27
@treeowl
Copy link
Contributor Author

treeowl commented Jul 23, 2019

I realized I can loosen some of the Newtype constraints to Coercible ones without losing the inference that is this module's raison d'être.

@treeowl
Copy link
Contributor Author

treeowl commented Jul 23, 2019

By the way: the module name is totally up for grabs.

@treeowl
Copy link
Contributor Author

treeowl commented Jul 23, 2019

Once #13 is merged, I can replace the implementations in this module with calls to the more-polymorphic versions.

@sjakobi
Copy link
Collaborator

sjakobi commented Jul 24, 2019

How about we actually replace the existing combinators in CoercibleUtils with these?

I'm mostly concerned about offering two very similar APIs in the same package – I think that would be confusing.

Thoughts @chessai?

@chessai
Copy link
Collaborator

chessai commented Jul 24, 2019

The competing APIs thing seems like it might be a problem to me. The new API requires a generic instance, which I'm not sure is that much of a problem. It also doesn't work with UnliftedNewtypes.

I'm tempted to say keep both for now. But I'm not sure yet.

Also make some of the implementations more readable, and export
`pack` and `unpack`.
@treeowl
Copy link
Contributor Author

treeowl commented Jul 24, 2019

@sjakobi and @chessai, I think this is ready to go wherever you want it to go and with whatever name you wish to give it. Can we get it done? Creating yet another package with such similar functionality seems a bit silly to me, but I guess I can do that if necessary....

@sjakobi
Copy link
Collaborator

sjakobi commented Jul 24, 2019

As I'm not aware of any actual users of the combinators in CoercibleUtils, I don't see the point of keeping them around if we now have better versions of them. UnliftedNewtypes hasn't even been released yet.

If anyone wants to get the old ones back, I'm sure we can figure something out.

Unless you protest @chessai, I'd merge this as-is and move things into CoercibleUtils.

@chessai chessai merged commit 7cfa617 into love-haskell:master Jul 24, 2019
@chessai
Copy link
Collaborator

chessai commented Jul 24, 2019

Ok. Agreed. My unlifted newtypes criticism wasnt very sensical.

@treeowl
Copy link
Contributor Author

treeowl commented Jul 24, 2019

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants