From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Kangas Newsgroups: gmane.emacs.bugs Subject: bug#36981: 26.2; request: add searching by package name to list-packages Date: Thu, 19 Sep 2019 01:17:43 +0200 Message-ID: References: <87ef0r396q.fsf@gmail.com> <87tv9mke73.fsf@gmail.com> <87woeh197j.fsf@gmail.com> <87a7b7164f.fsf@gmail.com> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000f7a1bc0592dc0b87" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="51124"; mail-complaints-to="usenet@blaine.gmane.org" Cc: 36981@debbugs.gnu.org, ndame , =?UTF-8?Q?=C5=A0t=C4=9Bp=C3=A1n_?= =?UTF-8?Q?N=C4=9Bmec?= To: Federico Tedin Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Sep 19 01:19:47 2019 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1iAjEf-000D9T-IK for geb-bug-gnu-emacs@m.gmane.org; Thu, 19 Sep 2019 01:19:45 +0200 Original-Received: from localhost ([::1]:35890 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iAjEd-0000EO-43 for geb-bug-gnu-emacs@m.gmane.org; Wed, 18 Sep 2019 19:19:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:33037) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iAjDz-0008J4-OR for bug-gnu-emacs@gnu.org; Wed, 18 Sep 2019 19:19:05 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iAjDy-0002Fz-DM for bug-gnu-emacs@gnu.org; Wed, 18 Sep 2019 19:19:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:46139) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iAjDy-0002Fp-9l for bug-gnu-emacs@gnu.org; Wed, 18 Sep 2019 19:19:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1iAjDy-0003ag-3l for bug-gnu-emacs@gnu.org; Wed, 18 Sep 2019 19:19:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Kangas Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 18 Sep 2019 23:19:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 36981 X-GNU-PR-Package: emacs Original-Received: via spool by 36981-submit@debbugs.gnu.org id=B36981.156884868313709 (code B ref 36981); Wed, 18 Sep 2019 23:19:02 +0000 Original-Received: (at 36981) by debbugs.gnu.org; 18 Sep 2019 23:18:03 +0000 Original-Received: from localhost ([127.0.0.1]:54959 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iAjD0-0003Ym-GI for submit@debbugs.gnu.org; Wed, 18 Sep 2019 19:18:03 -0400 Original-Received: from mail-pf1-f193.google.com ([209.85.210.193]:33485) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iAjCy-0003YK-Gs for 36981@debbugs.gnu.org; Wed, 18 Sep 2019 19:18:01 -0400 Original-Received: by mail-pf1-f193.google.com with SMTP id q10so1002105pfl.0 for <36981@debbugs.gnu.org>; Wed, 18 Sep 2019 16:18:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vxtH8HREWfOqblm+A8S0YHnrZQSjgJ8gcGsjtAl8jz4=; b=q0Y8HPHP47CRxPImYWYD94sm8qQrpCTBBUws5hcZOUpG+iWDqPswZXMMTJ75/yNafw oLqHDTjWbiz4kg5+dGL8MgJv+NWROBh/sT+/Zwq1nHQ1YffBItpWjg5mMx/GAT4PQdK9 oVQO2bVSzOYlTz+vcR8wOasXfs6pWHx93q3sHahXmAsCuxFAYa5bBvknoekFNq30SnCw XSM+GlI0LAf3IxgNEWoXzDcuqpt5Psvo+yelB8trbScdz1KIlfAZ7gKDmwlr6VQ8Hb5d sDn9p454jmER5YJK/CtSFl0YVxjtUvsWOYfYo+jPe5ed6NGFP0RlboqztR9g+N+etn9T gmFQ== X-Gm-Message-State: APjAAAWQ1lRV4N/lWMN5p2M5pnZhgwdMucw5BPh6DQcjhuJDdZzcoxhQ 8PWFiYquBLPVy2RLdQpkjFNl422950O0iAIx40o= X-Google-Smtp-Source: APXvYqzURm4XosX8ebZIXqGFBvnjPqCpmeE4wDdPwScUnuxghDzLmVSdBvmQZAN5pnXr9shh4t5Osu+zkWSiFUQpZ9U= X-Received: by 2002:aa7:8009:: with SMTP id j9mr6822622pfi.107.1568848674790; Wed, 18 Sep 2019 16:17:54 -0700 (PDT) In-Reply-To: <87a7b7164f.fsf@gmail.com> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.51.188.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:166694 Archived-At: --000000000000f7a1bc0592dc0b87 Content-Type: text/plain; charset="UTF-8" Hi Federico, Here's a more detailed review of your patch. Federico Tedin writes: > Subject: [PATCH 1/1] Search packages by name in list-packages (Bug#36981) Perhaps instead: "Filter packages by name in list-packages." > * lisp/emacs-lisp/package.el (package-menu-search): Added a new > function that allows filtering packages by name. Or, shorter: "New function to filter packages by name." > (package-menu--generate): Show full packages list with 'q' if current > list has been filtered by name as well. > (package-menu-mode-map): Bind 's' to package-menu-search. > * test/lisp/emacs-lisp/package-tests.el (package-test-list-search): > Added a test for package-menu-search. We prefer the imperative rather than the past tense in commit messages, so that should just be "Add". > +Search packages by name (@code{package-menu-search}). This prompts Suggest to change "Search" to "Filter". > +(defun package-menu-search (name) Suggest to rename to package-menu-filter-by-name > + "Filter the *Packages* buffer. I suggest "Filter the \"*Packages\" buffer by NAME. > + (interactive (list (read-from-minibuffer "Package name: "))) I suggest "Filter by name (regexp): " > + (if (or (not name) (string-empty-p name)) > + (package-show-package-list t nil) > + ;; Update `tabulated-list-entries' so that in contains all "in" -> "it" > + (let (matched) > + (dolist (entry tabulated-list-entries) > + (let* ((pkg-name-sym (package-desc-name (car entry))) > + (pkg-name (symbol-name pkg-name-sym))) This is nitpicking, but perhaps it would be easier to read if you use only one variable here. That means to keep "pkg-name-sym" but rename it to "pkg-name", and... > + (when (string-match name pkg-name) ... changing this to (when (string-match name (symbol-name pkg-name)) That would be preference, anyways. > +(ert-deftest package-test-list-search () > + "Ensure package list is filtered correctly by package name." > + (with-package-test () > + (let ((buf (package-list-packages))) > + (package-menu-search "tetris") > + (should (= (length tabulated-list-entries) 1)) > + (should (eq (package-desc-name (caar tabulated-list-entries)) 'tetris)) > + (kill-buffer buf)))) Wouldn't it be better to verify the buffer contents here? That way we it's less coupled to the implementation of tabulated-list-mode. Best regards, Stefan Kangas --000000000000f7a1bc0592dc0b87 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Federico,

Here's a more detailed review of y= our patch.

Federico Tedin <federicotedin@gmail.com> writes:

> Subject: [PATCH 1/= 1] Search packages by name in list-packages (Bug#36981)

Perhaps inst= ead: "Filter packages by name in list-packages."

> * li= sp/emacs-lisp/package.el (package-menu-search): Added a new
> functio= n that allows filtering packages by name.

Or, shorter: "New fun= ction to filter packages by name."

> (package-menu--generate= ): Show full packages list with 'q' if current
> list has bee= n filtered by name as well.
> (package-menu-mode-map): Bind 's= 9; to package-menu-search.
> * test/lisp/emacs-lisp/package-tests.el = (package-test-list-search):
> Added a test for package-menu-search.
We prefer the imperative rather than the past tense in commit message= s,
so that should just be "Add".

> +Search packages = by name (@code{package-menu-search}).=C2=A0 This prompts

Suggest to = change "Search" to "Filter".

> +(defun packag= e-menu-search (name)

Suggest to rename to package-menu-filter-by-nam= e

> + =C2=A0"Filter the *Packages* buffer.

I suggest = "Filter the \"*Packages\" buffer by NAME.

> + =C2= =A0(interactive (list (read-from-minibuffer "Package name: ")))
I suggest "Filter by name (regexp): "

> + =C2=A0(= if (or (not name) (string-empty-p name))
> + =C2=A0 =C2=A0 =C2=A0(pac= kage-show-package-list t nil)
> + =C2=A0 =C2=A0;; Update `tabulated-l= ist-entries' so that in contains all

"in" -> "= it"

> + =C2=A0 =C2=A0(let (matched)
> + =C2=A0 =C2=A0 = =C2=A0(dolist (entry tabulated-list-entries)
> + =C2=A0 =C2=A0 =C2=A0= =C2=A0(let* ((pkg-name-sym (package-desc-name (car entry)))
> + =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (pkg-name (symbol-name pkg-na= me-sym)))

This is nitpicking, but perhaps it would be easier to read= if you use
only one variable here.=C2=A0 That means to keep "pkg-n= ame-sym" but rename it
to "pkg-name", and...

> = + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(when (string-match name pkg-name)
<= br>... changing this to (when (string-match name (symbol-name pkg-name))
That would be preference, anyways.

> +(ert-deftest package-t= est-list-search ()
> + =C2=A0"Ensure package list is filtered co= rrectly by package name."
> + =C2=A0(with-package-test ()
>= ; + =C2=A0 =C2=A0(let ((buf (package-list-packages)))
> + =C2=A0 =C2= =A0 =C2=A0(package-menu-search "tetris")
> + =C2=A0 =C2=A0 = =C2=A0(should (=3D (length tabulated-list-entries) 1))
> + =C2=A0 =C2= =A0 =C2=A0(should (eq (package-desc-name (caar tabulated-list-entries)) = 9;tetris))
> + =C2=A0 =C2=A0 =C2=A0(kill-buffer buf))))

Wouldn= 't it be better to verify the buffer contents here?=C2=A0 That way weit's less coupled to the implementation of tabulated-list-mode.
Best regards,
Stefan Kangas
--000000000000f7a1bc0592dc0b87--