Re: [PATCH 1/6] emacs: Add 'guix-packages-by-location' command.

[Ludovic Courtès]

Alex Kost <address@hidden> skribis:

> * emacs/guix-main.scm (%package-location-table): New variable.
> (package-location-table, package-locations, packages-by-location): New
> procedures.
> (%patterns-makers): Add 'location' search type.
> * emacs/guix-messages.el (guix-message-packages-by-location): New procedure.
> (guix-messages): Use it.
> * emacs/guix-read.el (guix-package-locations)
> (guix-read-package-location): New procedures.
> * emacs/guix-ui-package.el (guix-packages-by-location): New command.
> * doc/emacs.texi (Emacs Commands): Document it.

[...]

> address@hidden M-x guix-packages-by-location
> +Display package(s) placed in the specified location.

s/location/source file/ IIUC.  Also, this must be a relative file name
like “gnu/packages/emacs.scm”, not just “emacs.scm”, right?  It would be
nice to clarify this here.

> +(define %package-location-table
> +  (delay
> +    (let ((table (make-hash-table
> +                  ;; Rough guess about a number of locations: it is
> +                  ;; about 10 times less than the number of packages.
> +                  (euclidean/ (vlist-length (package-vhash)) 10))))
> +      ;; XXX Actually, 'for-each-package' is needed but there is no such yet.
> +      (fold-packages
> +       (lambda (package _)
> +         (let* ((location (location-file (package-location package)))
> +                (packages (or (hash-ref table location) '())))
> +           (hash-set! table location (cons package packages))))
> +       #f)
> +      table)))

For the record (and maybe for a future patch, because the rest of the
file already uses the idiom above anyway), I find it somewhat “better”
to (1) use vhashes (easier to reason about), and (2) define lookup
procedures instead of exposing the data structure (no need to document
the data structure, can be changed at will, etc.).

That is, I would rather write:

  (define packages-in-source-file
    (let ((table (delay (fold-packages
                         (lambda (package table)
                           (match (and=> (package-location package)
                                         location-file)
                             (#f table)
                             (file (vhash-cons file package table))))
                         vlist-null))))
      (lambda (file)
        "Return the (possibly empty) list of packages defined in FILE."
        (vhash-fold* cons '() file (force table)))))

WDYT?

> +(define (packages-by-location location)
> +  "Return a list of packages placed in LOCATION."
> +  (hash-ref (package-location-table) location))

Again use “file” here since it’s a file name, not a <location> record,
that’s expected.

Also: “Return _the_ list”.

Otherwise LGTM.

Thanks!

Ludo’.