Hi,

sorry for taking so long to answer!

On Sat, May 2, 2015 at 2:48 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> Subject: [PATCH] import: hackage: Refactor parsing code and add new option.
>>
>> * guix/import/cabal.scm: New file.
>>
>> * guix/import/hackage.scm: Update to use the new Cabal parsing module.
>>
>> * tests/hackage.scm: Update tests for private functions.
>>
>> * guix/scripts/import/hackage.scm: Add new '--cabal-environment' option.
>>
>> * doc/guix.texi: ... and document it.
>>
>> * Makefile.am (MODULES): Add 'guix/import/cabal.scm',
>>   'guix/import/hackage.scm' and 'guix/scripts/import/hackage.scm'.
>>   (SCM_TESTS): Add 'tests/hackage.scm'.
>
> No newlines between entries.

Done.

 [...]

> This procedure is intimidating.  I think this is partly due to its
> length, to the big let-values, the long identifiers, the many local
> variables, nested binds, etc.

Ok, this procedure has now ... disappeared ... or rather it is now
hidden in a huge, but invisible macro ;-)
I've added support for braces delimited blocks.  In so doing the
complexity of an ad-hoc solution increased further and decided that it
was time to study (and use) a proper parser.

But, a couple of words on your remarks:

- Thanks to your comment about long list of local variables I
(re-)discovered the (test => expr) form of cond clauses. Very useful!
- The nested use of the >>= function didn't look nice and the reason
is that it is really meant as a way to sequence monadic functions as
in (>>= m f1 f2 ...).  Unfortunately the current version of >>= in
guile only accepts 2 arguments (1 function), hence the nesting.  It
would be nice to correct that :-)

In any case, I had to give up with the state monad because the lalr
parser in Guile doesn't play nice with the functional programming
paradigm.

>> +(define-record-type <cabal-package>
>> +  (make-cabal-package name version license home-page source-repository
>> +                      synopsis description
>> +                      executables lib test-suites
>> +                      flags eval-environment)
>> +  cabal-package?
>> +  (name   cabal-package-name)
>> +  (version cabal-package-version)
>> +  (license cabal-package-license)
>> +  (home-page cabal-package-home-page)
>> +  (source-repository cabal-package-source-repository)
>> +  (synopsis cabal-package-synopsis)
>> +  (description cabal-package-description)
>> +  (executables cabal-package-executables)
>> +  (lib cabal-package-library) ; 'library' is a Scheme keyword
>
> There are no keyboards in Scheme.  :-)

??

> [...]
>
>> +  (define (impl haskell)
>> +    (let* ((haskell-implementation (or (assoc-ref env "impl") "ghc"))
>> +           (impl-rx-result-with-version
>> +            (string-match "([a-zA-Z0-9_]+)-([0-9.]+)" haskell-implementation))
>> +           (impl-name (or (and=> impl-rx-result-with-version
>> +                                 (cut match:substring <> 1))
>> +                          haskell-implementation))
>> +           (impl-version (and=> impl-rx-result-with-version
>> +                                (cut match:substring <> 2)))
>> +           (cabal-rx-result-with-version
>> +            (string-match "([a-zA-Z0-9_-]+) *([<>=]+) *([0-9.]+) *" haskell))
>> +           (cabal-rx-result-without-version
>> +            (string-match "([a-zA-Z0-9_-]+)" haskell))
>> +           (cabal-impl-name (or (and=> cabal-rx-result-with-version
>> +                                       (cut match:substring <> 1))
>> +                                (match:substring
>> +                                 cabal-rx-result-without-version 1)))
>> +           (cabal-impl-version (and=> cabal-rx-result-with-version
>> +                                      (cut match:substring <> 3)))
>> +           (cabal-impl-operator (and=> cabal-rx-result-with-version
>> +                                       (cut match:substring <> 2)))
>> +           (comparison (and=> cabal-impl-operator
>> +                              (cut string-append "string" <>))))
>
> Again I feel we need one or more auxiliary procedures and/or data types
> here to simplify this part (fewer local variables), as well as shorter
> identifiers.  WDYT?


I've added two help functions to make it easier to read.

> The existing tests here are fine, but they are more like integration
> tests (they test the whole pipeline.)  Maybe it would be nice to
> directly exercise ‘read-cabal’ and ‘eval-cabal’ individually?

It is true that the tests are for the whole pipeline, but they catch
most of the problems (problems in any function along the chain) with
the smallest number of tests :-). I'm not very keen in doing fine
grained testing. Sorry.

I've removed the test with TABs because the Cabal documentation says
explicitly that they are not allowed.
https://www.haskell.org/cabal/users-guide/developing-packages.html#package-descriptions

I've changed the second test to check the use of braces (multi-line
values have still to be indented).

Regards,
Fede