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