Hi Federico,

Here's a more detailed review of your patch.

Federico Tedin <federicotedin@gmail.com> 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