snoyberg / mono-traversable

Type classes for mapping, folding, and traversing monomorphic containers
155 stars 64 forks source link

`singleton` not implemented for `NonEmpty`; change constraint #156

Closed gittywithexcitement closed 6 years ago

gittywithexcitement commented 6 years ago

Problem:

singleton is not implemented for NonEmpty This does not make sense; obviously you can create a NonEmpty from a single element.

Details:

  1. singleton has the constraint IsSequence seq.
  2. Monoid is a superclass of IsSequence
  3. NonEmpty is not an instance Monoid. This makes sense because there's no valid mempty.
  4. Thus singleton is not implemented for NonEmpty. This does not make sense because you can obviously create a valid NonEmpty container from a single element. The workaround is to use not-well-known functions like opoint or pure to create a NonEmpty. This makes my code less readable, because those functions are not well known (opoint) or not typically used to create a container (pure).

Solution:

Change singleton constraint to MonoPointed. That's all.

The implementation of singleton is: singleton = opoint which only requires a MonoPointed constraint, as opoint is a function of that type class. NonEmpty already is an instance of MonoPointed, so after changing the constraint, singleton will be implemented for NonEmpty.

snoyberg commented 6 years ago

Sounds fine to me, want to send a PR?

On Mon, Jan 15, 2018 at 7:44 PM, Daniel Killebrew notifications@github.com wrote:

Problem:

singleton is not implemented for NonEmpty This does not make sense; obviously you can create a NonEmpty from a single element. Details:

  1. singleton has the constraint IsSequence seq.
  2. Monoid is a superclass of IsSequence
  3. NonEmpty is not an instance Monoid. This makes sense because there's no valid mempty.
  4. Thus singleton is not implemented for NonEmpty. This does not make sense because you can obviously create a valid NonEmpty container from a single element. The workaround is to use not-well-known functions like opoint or pure to create a NonEmpty. This makes my code less readable, because those functions are not well known (opoint) or not typically used to create a container (pure).

Solution:

Change singleton constraint to MonoPointed. That's all.

The implementation of singleton is: singleton = opoint which only requires a MonoPointed constraint, as opoint is a function of that type class. NonEmpty already is an instance of MonoPointed, so after changing the constraint, singleton will be implemented for NonEmpty.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/snoyberg/mono-traversable/issues/156, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBB7GyGERTGO3y6wUXX1ekvRpxb9aXks5tK455gaJpZM4RewJU .

gittywithexcitement commented 6 years ago

Will do.

I see that singleton is in Data.Sequence, but MonoPointed is in Data.MonoTraversable. Thus, it would make sense to move singleton, given the updated constraint, but that would break user code. What do you suggest?

snoyberg commented 6 years ago

What's the motivation for moving it? opoint already exists in Data.MonoTraversable, and singleton is intended to complete the Data.Sequence API for compatibility with libraries like text and bytestring.

On Mon, Jan 15, 2018 at 8:01 PM, Daniel Killebrew notifications@github.com wrote:

Will do.

I see that singleton is in Data.Sequence, but MonoPointed is in Data.MonoTraversable. Thus, it would make sense to move singleton, given the updated constraint, but that would break user code. What do you suggest?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/snoyberg/mono-traversable/issues/156#issuecomment-357752826, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBBw-S3Dw79n9aXzWA30xBYnnxulK4ks5tK5J_gaJpZM4RewJU .

gittywithexcitement commented 6 years ago

I see. Given that, I see no reason to move it.

gittywithexcitement commented 6 years ago

Thanks for the super fast feedback!