commercialhaskell / rio

A standard library for Haskell
Other
838 stars 54 forks source link

exporting 'empty' for 'class Applicative f => Alternative f' #192

Closed ekalosak closed 2 years ago

ekalosak commented 5 years ago

Prelude wasn't exporting empty for class Alternative f so this one-line change adds that to the Prelude.hs re-export of the Control.Applicative module.

snoyberg commented 5 years ago

We've avoided the export till now because empty is such a common identifier in the ecosystem and exposing it from a prelude can cause conflicts.

On Wed, Jun 26, 2019, 10:10 PM Eric Kalosa-Kenyon notifications@github.com wrote:

Prelude wasn't exporting empty for class Alternative f so this one-line change adds that to the Prelude.hs re-export of the Control.Applicative module.

You can view, comment on, or merge this pull request online at:

https://github.com/commercialhaskell/rio/pull/192 Commit Summary

  • exporting 'empty' for 'class Applicative f => Alternative f'

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/commercialhaskell/rio/pull/192?email_source=notifications&email_token=AAAMCB4ZOHAUJFZISYGO3FDP4O5JRA5CNFSM4H3VHKE2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G34LXVA, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAMCBYGG25IAUQ4U6UGATLP4O5JRANCNFSM4H3VHKEQ .

ekalosak commented 5 years ago

That makes sense - I wonder if then it's not worth changing Alternative's definition? Are there other creative solutions or should we expect an import Control.Applicative (empty) when it's needed?

akhra commented 5 years ago

Changing the definition is a non-starter, we're not going to break compatibility with base.

Adding an alternate binding e.g. aempty = Alternative.empty and exposing that is an option; and though in general we try not to introduce original idioms (preferring established standards), there's precedent in stuff like fromStrictBytes. A quick stackage-hoogle search turns up no existing aempty so I'm tentatively in favor of going this way. @snoyberg ?

snoyberg commented 5 years ago

Exporting aempty seems fine. Keep in mind that this will work for using empty, but it you want to write a new typeclass instance you'll still have to deal with extra imports.

akhra commented 5 years ago

Good point -- IMO that's fine, but let's make sure the haddocks for aempty are clear that it's an alias.

ekalosak commented 5 years ago

(the force pushes were to reword commit messages, nothing else.)

snoyberg commented 5 years ago

The Haddocks are still incorrect, please see the build log. It would be best if you tried to build the docs locally