On Mon, Aug 03, 2015 at 01:48:45PM -0600, Peter Harpending wrote:
> Patch review time!
> 
> On Sun, Aug 02, 2015 at 11:26:14PM +0300, fr33domlo...@riseup.net wrote:
> 
> > +    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))

Close, but no cigar. :) Type mismatch and other infelicities. I
suggest using the Functor instance:

      return (fmap fromString murl)

> > +            _                           -> return Nothing

> Same thing here with aligning delimiters against variable-width
> blocks of text.

I almost agree. I'm on the fence about aligning delimiters against
variable-width blocks of text, but I definitely think nothing is
gained in doing so if there are intermediate lines that begin
far to the left of the aligned delimiter. I should stress this is
purely an issue of style, but hey, somebody has to make the rules....

Besides these purely-stylistic things I think the patch looks good.
I'll merge it. Thanks!

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Dev mailing list
Dev@lists.snowdrift.coop
https://lists.snowdrift.coop/mailman/listinfo/dev

Reply via email to