From: ludo@gnu.org (Ludovic Courtès)
To: Federico Beffa <beffa@ieee.org>
Cc: Guix-devel <guix-devel@gnu.org>
Subject: Re: hackage importer
Date: Fri, 05 Jun 2015 09:30:45 +0200 [thread overview]
Message-ID: <87sia6pq6y.fsf@gnu.org> (raw)
In-Reply-To: <CAKrPhPPYBZTcctDQFt4NSLpkNc36_VUMyg-pJay8DAswvUX5Vg@mail.gmail.com> (Federico Beffa's message of "Mon, 1 Jun 2015 17:20:06 +0200")
Howdy!
Federico Beffa <beffa@ieee.org> skribis:
> On Sat, May 2, 2015 at 2:48 PM, Ludovic Courtès <ludo@gnu.org> wrote:
[...]
>> 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.
Yay, good idea!
> 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 :-)
Sure, I have it in my to-do list. :-) I looked at it quickly and it
seems less trivial than initially envisioned though.
>>> +(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. :-)
>
> ??
Oops, these should have read “keywords”, not “keyboards.” :-)
>> 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
But are they actually used in practice?
> I've changed the second test to check the use of braces (multi-line
> values have still to be indented).
OK.
> From f422ea9aff3aa8425c80eaadf50628c24d54495a Mon Sep 17 00:00:00 2001
> From: Federico Beffa <beffa@fbengineering.ch>
> Date: Sun, 26 Apr 2015 11:22:29 +0200
> Subject: [PATCH] import: hackage: Refactor parsing code and add new options.
>
> * guix/import/cabal.scm: New file.
> * guix/import/hackage.scm: Update to use the new Cabal parsing module.
> * tests/hackage.scm: Update tests.
> * guix/scripts/import/hackage.scm: Add new '--cabal-environment' and '--stdin'
> options.
> * doc/guix.texi: ... and document them.
> * Makefile.am (MODULES): Add 'guix/import/cabal.scm',
> 'guix/import/hackage.scm' and 'guix/scripts/import/hackage.scm'.
> (SCM_TESTS): Add 'tests/hackage.scm'.
[...]
> +(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.
> +;; 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’ve only read through quickly, but the rest of the file looks lean and
clean!
> +(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.
Also please mention it in the docstring.
> -(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.
Thanks for the nice refactoring & new features!
Ludo’.
next prev parent reply other threads:[~2015-06-05 7:30 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-13 17:59 hackage importer Federico Beffa
2015-03-15 14:38 ` Ludovic Courtès
2015-03-15 22:29 ` Federico Beffa
2015-03-22 20:12 ` Federico Beffa
2015-03-26 13:09 ` Ludovic Courtès
2015-03-28 8:53 ` Federico Beffa
2015-03-29 13:58 ` Ludovic Courtès
2015-03-29 16:55 ` Federico Beffa
2015-03-31 13:33 ` Ludovic Courtès
2015-04-03 13:01 ` Federico Beffa
2015-04-05 18:24 ` Ludovic Courtès
2015-04-26 11:38 ` Federico Beffa
2015-05-02 12:48 ` Ludovic Courtès
2015-06-01 15:20 ` Federico Beffa
2015-06-05 7:30 ` Ludovic Courtès [this message]
2015-06-05 15:19 ` Federico Beffa
2015-06-09 7:38 ` Ludovic Courtès
2015-06-09 8:38 ` Federico Beffa
-- strict thread matches above, loose matches on Subject: below --
2015-04-26 11:52 Federico Beffa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sia6pq6y.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=beffa@ieee.org \
--cc=guix-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.