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

[ludo@gnu.org (Ludovic Courtès)]

Alex Kost <alezost@gmail.com> skribis:

> Ludovic Courtès (2016-04-05 23:38 +0300) wrote:
>
>> Alex Kost <alezost@gmail.com> 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.
>>
>> [...]
>>
>>> +@item 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"?

I don’t think we need to invent new phrases.  :-)

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

OK!  I’d just change the first sentence to:

  Display the package(s) located in the specified file.

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

Exposing data structures does not improve performance in this case.

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

Right, I had forgotten about that second use.

We could do something to have both lookup procedures close over the same
promise, for instance:

  (define-values (lookup1 lookup2)
    (let ((table (delay (fold-packages …))))
      (values (lambda (x) …)
              (lambda (y) …))))

This way there’s still only one ‘fold-packages’ call.

> I realize that what you suggest is clean and functional, but I prefer
> efficiency over purity...  But not in this case :-)

:-)

I agree we shouldn’t end up with inefficient solutions just because it
looks nicer to the eye, but in this case I think we’re doing OK.  ;-)

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

Fine with me, or ‘package-by-source-file’?

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

This is fine.

If you choose to take the ‘define-values’ approach above, I think you
can just list they keys already in the vhash:

  (define-values (package-by-something-file package-something-files)
    (let* ((table (delay …))
           (files (delay
                    (delete-duplicates
                      (vhash-fold (lambda (file _ result)
                                    (cons file result))
                                  '()
                                  (force table))))))
      (values …
              (lambda () (force files)))))
                              

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

No big deal, but “return a list” is imprecise IMO: it doesn’t just
return any list.  But I’m nitpicking like crazy, I should stop!  :-)

Thank you for your patience!

Ludo’.