Patch review time! On Sun, Aug 02, 2015 at 11:26:14PM +0300, fr33domlo...@riseup.net wrote: > From: fr33domlover <fr33domlo...@rel4tion.org> > > +import qualified Data.Text as T > +import Network.Libravatar (avatarUrl)
You are correct to follow the style already in the file, but it's not a
good idea to line up the `as X` or (foo, bar, baz) stuff. If someone
imports a really long module, the alignment of the entire import list
will be affected, thus polluting a diff. `qualified` is of constant
length, so lining up against `qualified` is okay.
> + let libravatar email = avatarUrl (Left email) False Nothing Nothing
> + avatarFinal <- liftIO $
> + case (userAvatar user, userEmail user, userEmail_verified user) of
> + (Just url, _, _) -> return $ Just url
> + (Nothing, Just email, True) -> do
> + murl <- libravatar $ T.unpack email
> + return $ murl >>= return . fromString
I am entirely sure this last line can be written more clearly. How about
return (Just (fromString murl))
> + _ -> return Nothing
Same thing here with aligning delimiters against variable-width blocks
of text. Overall, this is how I would write it
avatarFinal <- liftIO $
case (userAvatar user, userEmail user, userEmail_verified user) of
(Just url, _, _) -> return (Just url)
(Nothing, Just email, True) -> do
murl <- libravatar $ T.unpack email
return $ Just (fromString url)
_ -> return Nothing
That's all the commentary I have
Peter Harpending
pgpZGC0iAoSR1.pgp
Description: PGP signature
_______________________________________________ Dev mailing list Dev@lists.snowdrift.coop https://lists.snowdrift.coop/mailman/listinfo/dev
