-
Notifications
You must be signed in to change notification settings - Fork 10
add unwrapF and unF #15
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
base: master
Are you sure you want to change the base?
Conversation
This came up in slack just now. I mentioned that |
I see this All this are If you think it's worth adding tho what you think about the names? are this fine: |
Since we're implementing this as a straight coerce, we could drop the constraint entirely too? |
If we drop the constraint than it might be unsafe |
How would it be unsafe? |
src/Data/Newtype.purs
Outdated
@@ -33,6 +34,15 @@ class Newtype t a | t -> a where | |||
un :: forall t a. Newtype t a => (a -> t) -> t -> a | |||
un _ = unwrap | |||
|
|||
-- | Cheap version of `map unwrap`. Uses `unsafeCoerce` internally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we leave the map unwrap
?
Yes, it actually seems to be safe even for contravention functions |
There is no telling what a parameter is used for in the general case. Many libraries use |
@eric-corumdigital there is |
Yes, it works fine for Functors, but |
But what exactly is a problem? Can you point out some concrete example where the problems of |
@safareli |
Now that we have safe coercions we could, in the spirit of #22, constrain This would make the |
#22 was merged, so this can move forward. |
Is this needed now that we have |
Those functions could benefit from the functional dependency brought by the Newtype constraint, I’d be inclined to revisit this after 0.14 though: perhaps bare coerce will be enough for most cases and perhaps we’ll discover more helpful combinators for the rest. |
I've addressed the merge conflict. Are we still interested in adding this? |
CI is now failing with:
|
fixes #10