[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/6] emacs: Add 'guix-packages-by-location' command.
From: |
Ludovic Courtès |
Subject: |
Re: [PATCH 1/6] emacs: Add 'guix-packages-by-location' command. |
Date: |
Sat, 16 Apr 2016 00:23:07 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Alex Kost <address@hidden> skribis:
> 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"?
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’.
[PATCH 4/6] doc: emacs: Add "Locations" section., Alex Kost, 2016/04/04
[PATCH 5/6] emacs: Add interface for package locations., Alex Kost, 2016/04/04