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

Reexport Ap and change definition of foldMapA to use Ap #121

Closed
chessai opened this issue Nov 7, 2018 · 8 comments
Closed

Reexport Ap and change definition of foldMapA to use Ap #121

chessai opened this issue Nov 7, 2018 · 8 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed reexport Reexport something new refactoring

Comments

@chessai
Copy link

chessai commented Nov 7, 2018

current definition of foldMapA:

foldMapA :: forall b m f a . (Monoid b, Applicative m, Foldable f) => (a -> m b) -> f a -> m b
foldMapA f = foldr (\a mb -> liftA2 mappend (f a) mb) (pure mempty)

instead of using an explicit right fold, it might be better to use whatever foldMap is, using Data.Monoid.Ap:

newtype Ap f a = Ap { getAp :: f a }
  deriving (Functor, Applicative)

instance (Applicative f, Semigroup a) => Semigroup (Ap f a) where
  x <> y = liftA2 (<>) x y

instance (Applicative f, Monoid a) => Monoid (Ap f a) where
  mempty = pure mempty

foldMapA :: (Monoid b, Foldable t, Applicative f) => (a -> f b) -> t a -> f b
foldMapA f = getAp . foldMap (Ap . f)

definitions of foldMap are usually defined to be advantageous over the structure of a particular Foldable; using foldr is picking the direction of association, which seems unnecessary.

@chessai
Copy link
Author

chessai commented Nov 7, 2018

this also applies to foldMapM

@vrom911 vrom911 added enhancement New feature or request question Further information is requested labels Nov 8, 2018
@cronokirby
Copy link
Collaborator

This is seems like a great idea to me!

@chessai
Copy link
Author

chessai commented Nov 8, 2018

note that i believe base 4.13 is going to have foldMap', a strict variant of foldMap, so something like foldMapA' can just be defined in terms of this new function.

@chshersh chshersh added this to the v0.4.1: Improvements milestone Nov 9, 2018
@chshersh chshersh added this to To do in #3: Hero Academia (November, 2018) via automation Nov 9, 2018
@chshersh
Copy link
Contributor

chshersh commented Nov 9, 2018

@chessai This looks like a good improvement! Unfortunately, Ap was introduced in base-4.12 (GHC-8.6.1) and relude supports all GHC versions to GHC-8.0.2. so the only possible way to implement foldMapA in this version is to use -XCPP and have two function with same types but different solutions.

@chessai
Copy link
Author

chessai commented Nov 9, 2018

Yeah. We could also define it for this purpose and not export it. What i typed out wrt Ap is all that's needed.

@chshersh
Copy link
Contributor

chshersh commented Nov 9, 2018

@chessai Well, I'm actually okay with exporting Ap. This data type looks useful and can be put in this module either implemented manually or reexported from base:

And them foldMapA can use Ap from Relude.Monoid module.

@chshersh chshersh changed the title consider changing definition of foldMapA Reexport Ap and change definition of foldMapA to use Ap Nov 9, 2018
@chshersh chshersh added refactoring reexport Reexport something new and removed question Further information is requested labels Nov 9, 2018
@chessai
Copy link
Author

chessai commented Nov 9, 2018

in that case, for GHC pre-8.6, you might want to just copy the code for Ap completely, and for base >= 4.12 re-export it.

@chessai
Copy link
Author

chessai commented Nov 9, 2018

This is all the code related to Ap:

-- | This data type witnesses the lifting of a 'Monoid' into an
-- 'Applicative' pointwise.
--
-- @since 4.12.0.0
newtype Ap f a = Ap { getAp :: f a }
        deriving ( Alternative -- ^ @since 4.12.0.0
                 , Applicative -- ^ @since 4.12.0.0
                 , Enum        -- ^ @since 4.12.0.0
                 , Eq          -- ^ @since 4.12.0.0
                 , Functor     -- ^ @since 4.12.0.0
                 , Generic     -- ^ @since 4.12.0.0
                 , Generic1    -- ^ @since 4.12.0.0
                 , Monad       -- ^ @since 4.12.0.0
                 , MonadFail   -- ^ @since 4.12.0.0
                 , MonadPlus   -- ^ @since 4.12.0.0
                 , Ord         -- ^ @since 4.12.0.0
                 , Read        -- ^ @since 4.12.0.0
                 , Show        -- ^ @since 4.12.0.0
                 )

-- | @since 4.12.0.0
instance (Applicative f, Semigroup a) => Semigroup (Ap f a) where
        (Ap x) <> (Ap y) = Ap $ liftA2 (<>) x y

-- | @since 4.12.0.0
instance (Applicative f, Monoid a) => Monoid (Ap f a) where
        mempty = Ap $ pure mempty

-- | @since 4.12.0.0
instance (Applicative f, Bounded a) => Bounded (Ap f a) where
  minBound = pure minBound
  maxBound = pure maxBound

-- | @since 4.12.0.0
instance (Applicative f, Num a) => Num (Ap f a) where
  (+)         = liftA2 (+)
  (*)         = liftA2 (*)
  negate      = fmap negate
  fromInteger = pure . fromInteger
  abs         = fmap abs
  signum      = fmap signum

@chshersh chshersh added the help wanted Extra attention is needed label Nov 15, 2018
@chshersh chshersh added the good first issue Good for newcomers label Jan 11, 2019
@chshersh chshersh self-assigned this Feb 13, 2019
chshersh added a commit that referenced this issue Feb 16, 2019
vrom911 pushed a commit that referenced this issue Feb 17, 2019
* [#121] Reexport Ap and change foldMapA

Resolves #121

* Fix HLint and build for GHC-8.0.2

* Fix haddock and Monoid instance for Ap for GHC-8.0.2

* Fix Monoid instance for Ap again

* Fix foldMapA

* Update @SInCE versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed reexport Reexport something new refactoring
Projects
No open projects
Development

No branches or pull requests

4 participants