Re: [PATCH master 2/3] Add Haskell parser for "xm list --long"

[Michele Tartara]

On Mon, Feb 18, 2013 at 11:50 AM, Iustin Pop <ius...@google.com> wrote:

> On Wed, Feb 06, 2013 at 01:09:51PM +0100, Michele Tartara wrote:
> > In order to fetch precise information about the status of the VMs
> running in
> > Xen, we need to analyze the output of the "xm list --long" command.
> >
> > This commit adds the parser to do that, and its tests.
>
> Separate comment from the backtracking issue:
>
> > +-- | A parser for parsing generic config files written in the
> (LISP-like)
> > +-- format that is the output of the @xm list --long@ command.
> > +-- This parser only takes care of the syntactic parse, but does not care
> > +-- about the semantics.
> > +lispConfigParser :: Parser LispConfig
> > +lispConfigParser =
> > +  A.skipSpace *>
> > +    (   listConfigP
> > +    <|> doubleP
> > +    <|> stringP
> > +    )
> > +  <* A.skipSpace
> > +    where listConfigP =
> > +            (A.char '(' *> lispConfigParser `AC.manyTill` A.char ')')
> > +              >>= pure . LCList
> > +          doubleP = A.double >>= pure . LCDouble
> > +          stringP =
> > +            A.takeWhile1 (
> > +              \c -> (not . isSpace) c
> > +                    && (c /= ')')
> > +                    && (c /= '(')
> > +            ) >>= (pure . LCString . unpack )
>
> You're mixing here Applicative and Monad operations (>>= pure . …). It
> would be IMHO cleaner to use just the Applicative interface:
>
>   listConfigP = LCList <$> (A.char …)
>   doubleP = LCDouble <$> A.double
>   stringP = LCString . unpack <$> …
>
>
You're right. I'll change it.

Thanks,
Michele