Move Const to own module in Data.Functor.Const and enable PolyKinds
ClosedPublic

Authored by duairc on Dec 15 2015, 11:09 AM.

Details

Summary

Const from Control.Applicative can trivially be made kind-polymorphic in its second argument. There has been a Trac issue about this for nearly a year now. It doesn't look like anybody objects to it, so I figured I might as well make a patch.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
duairc updated this revision to Diff 5724.Dec 15 2015, 11:09 AM
duairc retitled this revision from to Make Const (Control.Applicative) kind polymorphic in its second argument.
duairc updated this object.
duairc edited the test plan for this revision. (Show Details)
duairc set the repository for this revision to rGHC Glasgow Haskell Compiler.
duairc updated the Trac tickets for this revision.
RyanGlScott added inline comments.
libraries/base/changelog.md
109

Enabling PolyKinds in Control.Applicative also makes WrappedMonad :: (k -> *) -> k -> *. Please mention this!

RyanGlScott added inline comments.Dec 15 2015, 11:24 AM
libraries/base/changelog.md
109

Also, WrappedArrow :: (k -> k1 -> *) -> k -> k1 -> *.

Do we want WrappedArrow and WrappedMonad to be kind-polymorphic as well? Should I change change the documentation or should I add kind signatures to WrappedMonad and WrappedArrow restricting them to *? Especially seeing as there is talk of splitting Const off into its own module Data.Functor.Const as well.

Or I could just make a patch that implements that (moving Const out of Control.Applicative) (but re-exporting it obviously), and then if people want to make WrappedMonad kind-polymorphic later they can do that then.

Hm, it would probably be best to avoid changing WrappedMonad and WrappedArrow for now, especially since there's been no discussion of it on the libraries mailing list.

Also, you're right in that there's been talk of moving Const to its own module. Ideally, that would be part of D1543, but the likelihood of that making it in time for 8.0 is rapidly diminishing. The Data.Functor.Const part of the proposal, however, doesn't need to involve transformers, so if you want to make this patch encompass moving Const to Data.Functor.Const, I say go for it.

Make sure to do these two things:

  1. On this Diff, put Trac #11135 as a related issue so that we can cross Data.Functor.Const off the checklist once this is merged
  2. Add a comment to Trac #11135 saying that you're tackling this part of the proposal
duairc updated this revision to Diff 5739.Dec 15 2015, 4:43 PM
duairc retitled this revision from Make Const (Control.Applicative) kind polymorphic in its second argument to Move Const to own module in Data.Functor.Const and enable PolyKinds.
duairc edited edge metadata.
duairc updated the Trac tickets for this revision.
duairc updated this revision to Diff 5743.Dec 15 2015, 5:17 PM
duairc edited edge metadata.

I accidentally mixed this up with the changes I'm making to Const in D1626. This keeps them orthogonal.

duairc updated this revision to Diff 5745.Dec 15 2015, 5:48 PM
duairc edited edge metadata.

Forgot to update base.cabal

ekmett accepted this revision.Dec 18 2015, 7:12 PM
ekmett edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 18 2015, 7:12 PM
hvr requested changes to this revision.Dec 19 2015, 2:35 AM
hvr edited edge metadata.
hvr added inline comments.
libraries/base/Data/Functor/Const.hs
34

Why this annotation here? Const was available already before 4.9 in base from Control.Applicative...

This revision now requires changes to proceed.Dec 19 2015, 2:35 AM
duairc updated this revision to Diff 5850.Dec 19 2015, 7:57 AM
duairc edited edge metadata.

Removed erroneous @since annotation on Const data type

hvr accepted this revision.EditedDec 19 2015, 8:00 AM
hvr edited edge metadata.

LGTM (except for that minor typo in the changelog, and assuming this actually validates...)

libraries/base/changelog.md
112

# missing in front of 10039

This revision is now accepted and ready to land.Dec 19 2015, 8:00 AM
duairc updated this revision to Diff 5851.EditedDec 19 2015, 9:04 AM
duairc edited edge metadata.
duairc marked an inline comment as done.

The build failed because the updates to base.cabal from D1543 conflicted with this. I've rebased on top of the current git master branch and also fixed that typo in the changelog.

bgamari accepted this revision.Dec 20 2015, 4:43 AM
bgamari edited edge metadata.

Looks good to me. Thanks @duairc!

@duairc, could you let me know your authorship information (e.g. name and email address) so I can merge this?

This revision was automatically updated to reflect the committed changes.