Improve function signatures #44

Closed
tfausak opened this Issue Jun 28, 2014 · 12 comments

Comments

Projects
None yet
1 participant
@tfausak
Owner

tfausak commented Jun 28, 2014

Many of the functions take a lot of optional parameters. Instead of having a huge type signature, it seems like it would be better to do something like Yesod's settings types. Combining that with data-default could make for more understandable type signatures.

For instance, consider getSegmentLeaderboard. It has a lengthy type signature:

getSegmentLeaderboard :: Client -> SegmentId -> Maybe Char -> Maybe String -> Maybe String -> Maybe Bool -> Maybe Integer -> Maybe String -> Page -> PerPage -> IO (Either String [SegmentLeader])

By using a settings type with defaults, it could be this instead:

data LeaderboardSettings = ...
instance Default LeaderboardSettings where ...
getSegmentLeaderboard :: Client -> SegmentId -> LeaderboardSettings -> IO (Either String [SegmentLeader])

And then calling it would look a lot better:

getSegmentLeaderboard client 123 (def { gender = Just 'F', .. })

@tfausak tfausak added the enhancement label Jun 28, 2014

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jun 28, 2014

Owner

One problem with this approach is avoiding record collisions. I'd have to do something like what I'm doing for Strive.Objects. For instance, many of the settings will have page and perPage. That will cause the pretty type signatures to yield ugly usage. To wit:

import qualified Strive.Settings.LeaderboardSettings as LS
import qualified Strive.Settings.StarredSegmentSettings as SSS
getSegmentLeaderboard client 123 $ def { LS.page = Just 1, .. }
getStarredSegments client $ def { SSS.page = Just 2, .. }

This is a problem because it makes using the library a huge pain. You can no longer get everything you need with just import Strive. However, you can't really do that currently with all the Strive.Objects record collisions. Maybe I should take Yesod's approach and make all records unique by prefixing their names.

getSegmentLeaderboard client 123 $ def { leaderboardSettingsPage = Just 1, ...}
getStarredSegments client $ def { starredSegmentsSettingsPage = Just 2, .. }

That's just so damn verbose, though.

Owner

tfausak commented Jun 28, 2014

One problem with this approach is avoiding record collisions. I'd have to do something like what I'm doing for Strive.Objects. For instance, many of the settings will have page and perPage. That will cause the pretty type signatures to yield ugly usage. To wit:

import qualified Strive.Settings.LeaderboardSettings as LS
import qualified Strive.Settings.StarredSegmentSettings as SSS
getSegmentLeaderboard client 123 $ def { LS.page = Just 1, .. }
getStarredSegments client $ def { SSS.page = Just 2, .. }

This is a problem because it makes using the library a huge pain. You can no longer get everything you need with just import Strive. However, you can't really do that currently with all the Strive.Objects record collisions. Maybe I should take Yesod's approach and make all records unique by prefixing their names.

getSegmentLeaderboard client 123 $ def { leaderboardSettingsPage = Just 1, ...}
getStarredSegments client $ def { starredSegmentsSettingsPage = Just 2, .. }

That's just so damn verbose, though.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jun 28, 2014

Owner

As a corollary, prefixing record names would allow me to greatly reduce the number of modules. All of the athlete objects could be put directly in Strive.Objects.Athletes, for instance.

Owner

tfausak commented Jun 28, 2014

As a corollary, prefixing record names would allow me to greatly reduce the number of modules. All of the athlete objects could be put directly in Strive.Objects.Athletes, for instance.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jun 29, 2014

Owner

Ignoring record collisions, this is how buildAuthorizeUrl might look:

module Strive.Actions.Authentication
    ( buildAuthorizeUrl
    ) where

import Data.ByteString.Char8 (unpack)
import Data.Default (Default, def)
import Data.List (intercalate)
import Data.Monoid ((<>))
import Network.HTTP.Types (renderQuery, toQuery)

buildAuthorizeUrl :: Integer -> String -> Options -> String
buildAuthorizeUrl clientId redirectUrl options =
    "https://www.strava.com/oauth/authorize" <> unpack (renderQuery True query)
  where
    query = toQuery
        [ ("client_id", Just (show clientId))
        , ("redirect_url", Just redirectUrl)
        , ("response_type", Just "code")
        , ("approval_prompt", approvalPrompt options)
        , ("scope", fmap (intercalate ",") (scope options))
        , ("state", state options)
        ]

data Options = Options
    { approvalPrompt :: Maybe String
    , scope :: Maybe [String]
    , state :: Maybe String
    }

instance Default Options where
    def = Options
        { approvalPrompt = Nothing
        , scope = Nothing
        , state = Nothing
        }
Owner

tfausak commented Jun 29, 2014

Ignoring record collisions, this is how buildAuthorizeUrl might look:

module Strive.Actions.Authentication
    ( buildAuthorizeUrl
    ) where

import Data.ByteString.Char8 (unpack)
import Data.Default (Default, def)
import Data.List (intercalate)
import Data.Monoid ((<>))
import Network.HTTP.Types (renderQuery, toQuery)

buildAuthorizeUrl :: Integer -> String -> Options -> String
buildAuthorizeUrl clientId redirectUrl options =
    "https://www.strava.com/oauth/authorize" <> unpack (renderQuery True query)
  where
    query = toQuery
        [ ("client_id", Just (show clientId))
        , ("redirect_url", Just redirectUrl)
        , ("response_type", Just "code")
        , ("approval_prompt", approvalPrompt options)
        , ("scope", fmap (intercalate ",") (scope options))
        , ("state", state options)
        ]

data Options = Options
    { approvalPrompt :: Maybe String
    , scope :: Maybe [String]
    , state :: Maybe String
    }

instance Default Options where
    def = Options
        { approvalPrompt = Nothing
        , scope = Nothing
        , state = Nothing
        }
@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jun 30, 2014

Owner

The Options type could also implement QueryLike.

instance QueryLike Options where
    toQuery options = toQuery
        [ ("approval_prompt", approvalPrompt options)
        , ("scope", scope options)
        , ("state", state options)
        ]

That would simplify the definition of query.

    query = toQuery
        [ ("client_id", Just (show clientId))
        , ("redirect_url", Just redirectUrl)
        , ("response_type", Just "code")
        ] <> toQuery options
Owner

tfausak commented Jun 30, 2014

The Options type could also implement QueryLike.

instance QueryLike Options where
    toQuery options = toQuery
        [ ("approval_prompt", approvalPrompt options)
        , ("scope", scope options)
        , ("state", state options)
        ]

That would simplify the definition of query.

    query = toQuery
        [ ("client_id", Just (show clientId))
        , ("redirect_url", Just redirectUrl)
        , ("response_type", Just "code")
        ] <> toQuery options
@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jul 1, 2014

Owner

Here's a more realistic example of how updateCurrentAthlete would look with everything fully qualified.

data UpdateCurrentAthleteOptions = UpdateCurrentAthleteOptions
    { updateCurrentAthleteCity    :: Maybe String
    , updateCurrentAthleteCountry :: Maybe String
    , updateCurrentAthleteSex     :: Maybe Char
    , updateCurrentAthleteState   :: Maybe String
    , updateCurrentAthleteWeight  :: Maybe Double
    }

instance QueryLike UpdateCurrentAthleteOptions where
    toQuery options = toQuery
        [ ("city",    updateCurrentAthleteCity)
        , ("country", updateCurrentAthleteCountry)
        , ("sex",     fmap (: []) updateCurrentAthleteSex)
        , ("state",   updateCurrentAthleteState)
        , ("weight",  fmap show updateCurrentAthleteWeight)
        ]

updateCurrentAthlete :: Client
    -> UpdateCurrentAthleteOptions 
    -> IO (Either String DetailedAthlete)
updateCurrentAthlete client options = put client resource query
  where
    resource = "athlete"
    query = toQuery options

Which you would then use like this:

response <- updateCurrentAthlete client def
    { updateCurrentAthleteCity    = Just "Dallas"
    , updateCurrentAthleteCountry = Just "United States"
    , updateCurrentAthleteSex     = Just 'M'
    , updateCurrentAthleteState   = Just "TX"
    , updateCurrentAthleteWeight  = Just 72.57
    }

case response of
    Left error           -> fail error
    Right currentAthlete -> print (detailedAthleteCity currentAthlete)
Owner

tfausak commented Jul 1, 2014

Here's a more realistic example of how updateCurrentAthlete would look with everything fully qualified.

data UpdateCurrentAthleteOptions = UpdateCurrentAthleteOptions
    { updateCurrentAthleteCity    :: Maybe String
    , updateCurrentAthleteCountry :: Maybe String
    , updateCurrentAthleteSex     :: Maybe Char
    , updateCurrentAthleteState   :: Maybe String
    , updateCurrentAthleteWeight  :: Maybe Double
    }

instance QueryLike UpdateCurrentAthleteOptions where
    toQuery options = toQuery
        [ ("city",    updateCurrentAthleteCity)
        , ("country", updateCurrentAthleteCountry)
        , ("sex",     fmap (: []) updateCurrentAthleteSex)
        , ("state",   updateCurrentAthleteState)
        , ("weight",  fmap show updateCurrentAthleteWeight)
        ]

updateCurrentAthlete :: Client
    -> UpdateCurrentAthleteOptions 
    -> IO (Either String DetailedAthlete)
updateCurrentAthlete client options = put client resource query
  where
    resource = "athlete"
    query = toQuery options

Which you would then use like this:

response <- updateCurrentAthlete client def
    { updateCurrentAthleteCity    = Just "Dallas"
    , updateCurrentAthleteCountry = Just "United States"
    , updateCurrentAthleteSex     = Just 'M'
    , updateCurrentAthleteState   = Just "TX"
    , updateCurrentAthleteWeight  = Just 72.57
    }

case response of
    Left error           -> fail error
    Right currentAthlete -> print (detailedAthleteCity currentAthlete)
@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jul 1, 2014

Owner

I could go full typeclass and do this:

import Data.Default (Default, def)

data Athlete = Athlete
    { athleteName :: String
    , athletePicture :: String
    } deriving Show
data Club = Club
    { clubName :: String
    , clubPicture :: String
    } deriving Show

instance Default Athlete where 
    def = Athlete 
        { athleteName = ""
        , athletePicture = "" 
        }
instance Default Club where 
    def = Club 
        { clubName = ""
        , clubPicture = "" 
        }

class GetName a where
    getName :: a -> String
instance GetName Athlete where
    getName = athleteName
instance GetName Club where
    getName = clubName

class SetName a where 
    setName :: a -> String -> a
instance SetName Athlete where 
    setName athlete name = athlete { athleteName = name }
instance SetName Club where 
    setName club name = club { clubName = name }

class GetPicture a where 
    getPicture :: a -> String
instance GetPicture Athlete where 
    getPicture = athletePicture
instance GetPicture Club where 
    getPicture = clubPicture

class SetPicture a where 
    setPicture :: a -> String -> a
instance SetPicture Athlete where 
    setPicture athlete name = athlete { athletePicture = name }
instance SetPicture Club where 
    setPicture club name = club { clubPicture = name }

Then you could create stuff using a record-like syntax.

aClub :: Club
aClub = def
    `setName` "Fixed Touring"
    `setPicture` "http://fxt.io"

... I think I accidentally stumbled upon lenses.

Owner

tfausak commented Jul 1, 2014

I could go full typeclass and do this:

import Data.Default (Default, def)

data Athlete = Athlete
    { athleteName :: String
    , athletePicture :: String
    } deriving Show
data Club = Club
    { clubName :: String
    , clubPicture :: String
    } deriving Show

instance Default Athlete where 
    def = Athlete 
        { athleteName = ""
        , athletePicture = "" 
        }
instance Default Club where 
    def = Club 
        { clubName = ""
        , clubPicture = "" 
        }

class GetName a where
    getName :: a -> String
instance GetName Athlete where
    getName = athleteName
instance GetName Club where
    getName = clubName

class SetName a where 
    setName :: a -> String -> a
instance SetName Athlete where 
    setName athlete name = athlete { athleteName = name }
instance SetName Club where 
    setName club name = club { clubName = name }

class GetPicture a where 
    getPicture :: a -> String
instance GetPicture Athlete where 
    getPicture = athletePicture
instance GetPicture Club where 
    getPicture = clubPicture

class SetPicture a where 
    setPicture :: a -> String -> a
instance SetPicture Athlete where 
    setPicture athlete name = athlete { athletePicture = name }
instance SetPicture Club where 
    setPicture club name = club { clubPicture = name }

Then you could create stuff using a record-like syntax.

aClub :: Club
aClub = def
    `setName` "Fixed Touring"
    `setPicture` "http://fxt.io"

... I think I accidentally stumbled upon lenses.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jul 1, 2014

Owner

There are a few options for lenses: http://stackoverflow.com/questions/5767129/lenses-fclabels-data-accessor-which-library-for-structure-access-and-mutatio. Of the options presented there, I think I like fclabels the best. It has the fewest dependencies and seems like the most practical choice.

Owner

tfausak commented Jul 1, 2014

There are a few options for lenses: http://stackoverflow.com/questions/5767129/lenses-fclabels-data-accessor-which-library-for-structure-access-and-mutatio. Of the options presented there, I think I like fclabels the best. It has the fewest dependencies and seems like the most practical choice.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jul 1, 2014

Owner

Here's a subset of the previous example using fclabels:

{-# LANGUAGE TemplateHaskell #-}

import Data.Default (Default, def)
import Data.Label (mkLabel, set)

data Athlete = Athlete { _name :: String } deriving Show

instance Default Athlete where def = Athlete { _name = "Default Athlete" }

$(mkLabel ''Athlete)

Using this has the downside of inverting the builder.

anAthlete :: Athlete
anAthlete =
    set name "Taylor Fausak" $
    set name "Someone" $
    def
Owner

tfausak commented Jul 1, 2014

Here's a subset of the previous example using fclabels:

{-# LANGUAGE TemplateHaskell #-}

import Data.Default (Default, def)
import Data.Label (mkLabel, set)

data Athlete = Athlete { _name :: String } deriving Show

instance Default Athlete where def = Athlete { _name = "Default Athlete" }

$(mkLabel ''Athlete)

Using this has the downside of inverting the builder.

anAthlete :: Athlete
anAthlete =
    set name "Taylor Fausak" $
    set name "Someone" $
    def
@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jul 1, 2014

Owner

Hmm, using fclabels doesn't solve the naming collision problem.

-- Athlete.hs
{-# LANGUAGE TemplateHaskell #-}

module Athlete where

import Data.Default (Default, def)
import Data.Label (mkLabel)

data Athlete = Athlete
    { _name :: String
    , _id   :: Integer
    } deriving Show

$(mkLabel ''Athlete)

instance Default Athlete where
    def = Athlete
        { _name = ""
        , _id   = 0
        }
-- Gear.hs
{-# LANGUAGE TemplateHaskell #-}

module Gear where

import Data.Default (Default, def)
import Data.Label (mkLabel)

data Gear = Gear
    { _name :: String
    , _id   :: String
    } deriving Show

$(mkLabel ''Gear)

instance Default Gear where
    def = Gear
        { _name = ""
        , _id   = ""
        }
import Athlete
import Gear

import Data.Default (def)
import Data.Label (get)

anAthlete :: Athlete
anAthlete = def

aGear :: Gear
aGear = def

main :: IO ()
main = do
    print anAthlete
    print $ get Athlete.name anAthlete

    print aGear
    print $ get Gear.name aGear

Without the fully qualified names, I get this error:

Main.hs:18:17:
    Ambiguous occurrence `name'
    It could refer to either `Athlete.name',
                             imported from `Athlete' at Main.hs:3:1-14
                             (and originally defined at Athlete.hs:13:3-19)
                          or `Gear.name',
                             imported from `Gear' at Main.hs:4:1-11
                             (and originally defined at Gear.hs:13:3-16)
Owner

tfausak commented Jul 1, 2014

Hmm, using fclabels doesn't solve the naming collision problem.

-- Athlete.hs
{-# LANGUAGE TemplateHaskell #-}

module Athlete where

import Data.Default (Default, def)
import Data.Label (mkLabel)

data Athlete = Athlete
    { _name :: String
    , _id   :: Integer
    } deriving Show

$(mkLabel ''Athlete)

instance Default Athlete where
    def = Athlete
        { _name = ""
        , _id   = 0
        }
-- Gear.hs
{-# LANGUAGE TemplateHaskell #-}

module Gear where

import Data.Default (Default, def)
import Data.Label (mkLabel)

data Gear = Gear
    { _name :: String
    , _id   :: String
    } deriving Show

$(mkLabel ''Gear)

instance Default Gear where
    def = Gear
        { _name = ""
        , _id   = ""
        }
import Athlete
import Gear

import Data.Default (def)
import Data.Label (get)

anAthlete :: Athlete
anAthlete = def

aGear :: Gear
aGear = def

main :: IO ()
main = do
    print anAthlete
    print $ get Athlete.name anAthlete

    print aGear
    print $ get Gear.name aGear

Without the fully qualified names, I get this error:

Main.hs:18:17:
    Ambiguous occurrence `name'
    It could refer to either `Athlete.name',
                             imported from `Athlete' at Main.hs:3:1-14
                             (and originally defined at Athlete.hs:13:3-19)
                          or `Gear.name',
                             imported from `Gear' at Main.hs:4:1-11
                             (and originally defined at Gear.hs:13:3-16)
@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jul 1, 2014

Owner

However, using lens does solve the collision problem. They can even be different types! All you need is 5 language extensions and template Haskell.

{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE FunctionalDependencies #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE TypeSynonymInstances #-}

import Control.Lens (makeFields, view, (^.))
import Prelude hiding (id)

data Athlete = Athlete
    { _athleteId   :: Integer
    , _athleteName :: String
    } deriving Show

data Club = Club
    { _clubId   :: Integer
    , _clubName :: String
    } deriving Show

data Gear = Gear
    { _gearId   :: String
    , _gearName :: String
    } deriving Show

$(makeFields ''Athlete)
$(makeFields ''Club)
$(makeFields ''Gear)

That lets you use this wonderful syntax:

let athlete = Athlete 123 "Taylor Fausak"
print $ view name athlete -- "Taylor Fausak"
print $ athlete ^. id     -- 123

let club = Club 456 "Fixed Touring"
print $ view name club -- "Fixed Touring"
print $ club ^. id     -- 456

let gear = Gear "b789" "KT"
print $ view name gear -- "KT"
print $ gear ^. id     -- "b789"
Owner

tfausak commented Jul 1, 2014

However, using lens does solve the collision problem. They can even be different types! All you need is 5 language extensions and template Haskell.

{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE FunctionalDependencies #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE TypeSynonymInstances #-}

import Control.Lens (makeFields, view, (^.))
import Prelude hiding (id)

data Athlete = Athlete
    { _athleteId   :: Integer
    , _athleteName :: String
    } deriving Show

data Club = Club
    { _clubId   :: Integer
    , _clubName :: String
    } deriving Show

data Gear = Gear
    { _gearId   :: String
    , _gearName :: String
    } deriving Show

$(makeFields ''Athlete)
$(makeFields ''Club)
$(makeFields ''Gear)

That lets you use this wonderful syntax:

let athlete = Athlete 123 "Taylor Fausak"
print $ view name athlete -- "Taylor Fausak"
print $ athlete ^. id     -- 123

let club = Club 456 "Fixed Touring"
print $ view name club -- "Fixed Touring"
print $ club ^. id     -- 456

let gear = Gear "b789" "KT"
print $ view name gear -- "KT"
print $ gear ^. id     -- "b789"
@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Jul 1, 2014

Owner

If I want to go down that route, it would be easier to use declareFields.

$(declareFields [d|
    data Athlete = Athlete
        { _athleteId   :: Integer
        , _athleteName :: String
        } deriving Show |])
Owner

tfausak commented Jul 1, 2014

If I want to go down that route, it would be easier to use declareFields.

$(declareFields [d|
    data Athlete = Athlete
        { _athleteId   :: Integer
        , _athleteName :: String
        } deriving Show |])

@tfausak tfausak referenced this issue in tfausak/tfausak.github.io Jul 1, 2014

Closed

Write a post about lenses in Haskell #32

tfausak added a commit that referenced this issue Jul 5, 2014

Re #44; add options for getActivity action
I introduced a few annoying things:

-   In the readme, I have to do a qualified import of the lenses. This
    makes for the stupid "Strive.set Strive.field value" usage. Once all
    of the actions have options, the readme can be rewritten to avoid
    this.

-   The usage of defaults with lenses feels backwards. It would be
    better if it was like this:

        def $
            set field value $
            ...

-   It would be handy to use the lenses internally. That would require
    defining the classes separate from the instances. Then I could
    import the classes and use them in the options. That would make the
    QueryLike instance much cleaner:

        instance QueryLike GetActivityOptions where
            toQuery options =
                [ ( pack "include_all_efforts"
                  , Just (toStrict (encode (get includeAllEfforts options)))
                  )
                ]

-   Like #50, the lens instances for options really need to be generated
    automatically.

@tfausak tfausak self-assigned this Jul 5, 2014

@tfausak tfausak closed this Jul 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment