On Fri, Jun 5, 2015 at 9:30 AM, Ludovic Courtès <ludo@gnu.org> wrote:
>> 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
>
> But are they actually used in practice?

When I prepared the very first version of the importer I did find one
case among all the ones I tried. I believe that now that package has
been update to a new version and doesn't include TABs anymore.

> [...]
>
>> +(define (make-stack)
>> +  "Creates a simple stack closure.  Actions on the generated stack are
>> +requested by calling it with one of the following symbols as the first
>> +argument: 'empty?, 'push!, 'top, 'pop! and 'clear!.  The action 'push! is the
>> +only one requiring a second argument corresponding to the object to be added
>> +to the stack."
>> +  (let ((stack '()))
>> +    (lambda (msg . args)
>> +      (cond ((eqv? msg 'empty?) (null? stack))
>> +            ((eqv? msg 'push!) (set! stack (cons (first args) stack)))
>> +            ((eqv? msg 'top) (if (null? stack) '() (first stack)))
>> +            ((eqv? msg 'pop!) (match stack
>> +                                ((e r ...) (set! stack (cdr stack)) e)
>> +                                (_ #f)))
>> +            ((eqv? msg 'clear!) (set! stack '()))
>> +            (else #f)))))
>
> Fair enough.  :-)  I wonder what happens exactly when trying to return
> monadic values in the parser.

Given that the parser repeatedly calls the tunk generated by
'make-lexer' without passing any state or knowing anything about to
which monad it may belong to, I thought that it would not work.  But,
as you see, I'm new to Scheme, new to monads, and new to Lisp in
general.

>
>> +;; Stack to track the structure of nested blocks
>> +(define context-stack (make-stack))
>
> What about making it either a SRFI-39 parameter, or a parameter to
> ‘make-cabal-parser’?

I made it a parameter. Thanks for suggesting it! It made me realize
what they are really used for :-)
Do you think it is correct to say that they serve the purpose of
special variables in Lisp? (I'm looking a little bit into Common Lisp
as well.)

>> +(define* (hackage->guix-package package-name #:key
>> +                                (include-test-dependencies? #t)
>> +                                (read-from-stdin? #f)
>> +                                (cabal-environment '()))
>
> Instead of #:read-from-stdin?, what about adding a #:port argument?
> That way, it would either read from PORT, or fetch from Hackage.  That
> seems more idiomatic and more flexible.

Absolutely! Changed.

>
> Also please mention it in the docstring.

Done.

>
>> -(test-assert "hackage->guix-package test 3"
>> -  (eval-test-with-cabal test-cabal-3))
>> -
>> -(test-assert "conditional->sexp-like"
>> -  (match
>> -    (eval-cabal-keywords
>> -     (conditional->sexp-like test-cond-1)
>> -     '(("debug" . "False")))
>> -    (('and ('or ('string-match "darwin" ('%current-system)) ('not '#f)) '#t)
>> -     #t)
>> -    (x
>> -     (pk 'fail x #f))))
>
> I’m a bit scared when we add new code *and* remove tests.  ;-)
>
> Could you add a couple of representative tests?  I’m sure you ran tests
> by hand at the REPL, so it should be a matter of adding them in the file.

The reason for deleting the test is that that particular function
doesn't exist anymore. The functionality that it did provide is now
integrated in the parser. So, I've added one new test which exercises
'read-cabal' with a bunch of nested conditionals.

Thanks for the review!
Fede