Alex Kost <alez...@gmail.com> skribis: > 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"?
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’.