[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: hackage importer
From: |
Federico Beffa |
Subject: |
Re: hackage importer |
Date: |
Mon, 1 Jun 2015 17:20:06 +0200 |
Hi,
sorry for taking so long to answer!
On Sat, May 2, 2015 at 2:48 PM, Ludovic Courtès <address@hidden> 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
0001-import-hackage-Refactor-parsing-code-and-add-new-opt.patch
Description: Text Data
- Re: hackage importer,
Federico Beffa <=