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