From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Kost Subject: Re: [PATCH 1/6] emacs: Add 'guix-packages-by-location' command. Date: Wed, 06 Apr 2016 12:20:41 +0300 Message-ID: <874mbfxfdi.fsf@gmail.com> References: <1459799266-7426-1-git-send-email-alezost@gmail.com> <1459799266-7426-2-git-send-email-alezost@gmail.com> <87h9ffyeno.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:59499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anjdm-0004Zi-HF for guix-devel@gnu.org; Wed, 06 Apr 2016 05:20:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1anjdi-0004HW-Fl for guix-devel@gnu.org; Wed, 06 Apr 2016 05:20:46 -0400 List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Ludovic =?utf-8?Q?Court=C3=A8s?= Cc: guix-devel@gnu.org Ludovic Court=C3=A8s (2016-04-05 23:38 +0300) wrote: > Alex Kost 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 proced= ure. >> (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/=E2=80=A6scm" or files fr= om 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 =E2=80=9Cgnu/packages/emacs.scm=E2=80=9D, not just =E2=80=9Cemacs.sc= m=E2=80=9D, 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 suc= h 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 =E2=80=9Cbetter=E2=80=9D > 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=3D> (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 =E2=80=9Cfile=E2=80=9D here since it=E2=80=99s a file name, not= a record, > that=E2=80=99s expected. > Also: =E2=80=9CReturn _the_ list=E2=80=9D. OK. BTW, I have always used "Return a list" in other places, so it will break my convention :-) --=20 Alex