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

[Alex Kost]

Ludovic Courtès (2016-04-05 23:38 +0300) wrote:

> 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.

Actually, I don't like the name "source file" here as it sounds like it
can be an arbitrary file with packages, while it can be one of the files
recognized by 'fold-packages', i.e. "gnu/packages/…scm" or files from
GUIX_PACKAGE_PATH.  I think "location" is the right term here, so what
about "location file"?

> 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.

Right, what about the following clarification:

--8<---------------cut here---------------start------------->8---
@item M-x guix-packages-by-location
Display package(s) placed in the specified location file.  These files
usually have the following form: @file{gnu/packages/emacs.scm}, but
don't type them manually!  Press @key{TAB} to complete the file name.
--8<---------------cut here---------------end--------------->8---

>> +(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),

It's not a problem, I'd like to fix it now instead of making a future
patch.

> 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.).

Heh, I just like hash tables :-) Also I like to expose data structures
for efficiency.  The problem (well, not really a problem) is: along with
this procedure (to get packages by location file), I need another one
(to get a list of location files).

With the top-level hash table, I call 'fold-packages' only once and then
I just use this table.  With lookup procedures, 'fold-packages' has to
be called for each of them, which looks redundant to me.

I realize that what you suggest is clean and functional, but I prefer
efficiency over purity...  But not in this case :-) Since there are only
2 "lookup procedures" for package locations, it's only a bit
inefficient, so I can bear it and I'm going to make the changes that you
propose, thanks!

> That is, I would rather write:
>
>   (define packages-in-source-file

And again, I don't like the name 'packages-in-source-file', what about
'packages-by-location-file'?  (I prefer "by" over "in" as it is already
used in other procedure names: by-regexp, by-name, by-license, etc.)

>     (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?

This is great, thanks!  I always forget about 'vhash-fold*'.

So, as far as I see it, another "lookup procedure" will be:

  (define package-location-files
    (memoize
     (lambda ()
       "Return the list of file names of all package locations."
       (fold-packages
        (lambda (package result)
          (let ((file (location-file (package-location package))))
            (if (member file result)
                result
                (cons file result))))
        '()))))

OK?  Or is there a better way to write it?

>> +(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”.

OK.  BTW, I have always used "Return a list" in other places, so it will
break my convention :-)

-- 
Alex