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

Use Natural Transformations wherever possible #207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

milesfrain
Copy link
Contributor

This change makes the type signatures of NonEmptyArray functions more consistent with the other container types.

Here's an example of the previous inconsistencies:
https://pursuit.purescript.org/search?q=toUnfoldable

@hdgarrood
Copy link
Contributor

FWIW this is something that we explicitly decided against previously. There will be discussion on a closed issue or PR in this repo or potentially one of the other core library repos.

Comment on lines +247 to +250
tail :: NonEmptyArray ~> Array
tail = adaptMaybe A.tail

init :: forall a. NonEmptyArray a -> Array a
init :: NonEmptyArray ~> Array
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the most appropriate use, since the inner values are changing. Took inspiration from List library:
https://github.com/purescript/purescript-lists/blob/62900a56f6864638c952575dfd211a1cc55be7da/src/Data/List/NonEmpty.purs#L149-L153
By the same logic, we could also convert many others, such as head and tail of Array, as was done in this List PR:
purescript/purescript-lists#101
I wonder if this will add to confusion for beginners.

There's also some potentially misleading info in our definition of natural transformation.

A natural transformation is a mapping between type constructors of kind Type -> Type where the mapping operation has no ability to manipulate the inner values.

the Functor constraint is not enforced here; that the types are of kind Type -> Type is enough for our purposes.

So maybe that first paragraph should be changed to:

the mapping operation has no ability to manipulate the types of the inner values

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re reading the docs in a different way from what is intended there, because it’s not actually possible to write a function f ~> g which does manipulate the inner values in the way that is meant there, the compiler won’t let you.

In most cases, all that the “cannot manipulate the inner values” condition means is that every element of type a in the result also occurred in the input. So reverse, init, and even head are all perfectly valid natural transformations. An invalid one would be eg map (_ + 1), but of course that wouldn’t compile as you’d get something like “can’t match a0 with Int”.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this will add to confusion for beginners.

For what it is worth, the fact that head and reverse weren't defined via natural transformations confused this beginner. I was reading Jordan's reference and my first thought was "I bet reverse has a natural transformation in it's type signature". I wonder if we could use ~> in the code as an example of best practice (assuming that it is best practice) and put comments in such as "This type signature could be expressed as forall a. ...".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is a best practice, it's not really very significant whether you use ~> or not. I just think we should avoid it in the core libraries because it's another thing to learn, it's harder to search for in Pursuit, and because it's awkward when you have cases which don't quite fit into natural transformations like tail :: forall a. List a -> Maybe (List a); if you wanted to write that with ~> you'd need to use a type like tail :: List ~> Compose Maybe List which I think we'd all probably agree is not appropriate.

@hdgarrood
Copy link
Contributor

I wasn't aware that the lists library uses the ~> type synonym. I agree that these should be consistent, but I think we should change the lists library to remove uses of ~> rather than adding them here.

@JordanMartinez
Copy link
Contributor

I agree that these should be consistent, but I think we should change the lists library to remove uses of ~> rather than adding them here.

I agree with this, too. It avoids the issue of confusion that can arise if someone doesn't know what ~> is.

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.

4 participants