Subject: Re: bug#38424: [PATCH] Add new filter functions to Package Menu

From: Eli Zaretskii <[email-masked]>
Date: Sat, 30 Nov 2019 14:18:12 +0200

> From: Stefan Kangas <[email-masked]>
> Date: Fri, 29 Nov 2019 13:31:10 +0100
> 
> The attached patches adds new commands to filter the "*Packages*"
> buffer by version, status and archive.  (The first patch only adds new
> version list comparison predicates, something I needed to simplify the
> second patch.)

We deliberately didn't define the functions you are now adding, since
they are just one 'not' away.  Do they really simplify the callers so
much that we now want to add them?

> * doc/emacs/package.texi (Package Menu): Document it.

This tells nothing about the changes which aren't "documenting it".
(And, btw, what is "it" here is not clear at all.)

> -        (when (or (eq packages t) (memq name packages))
> +        (when (or (not packages) (memq name packages))
>            (dolist (pkg (cdr elt))
>              (when (package--has-keyword-p pkg keywords)
>                (push pkg info-list))))))
> @@ -2950,7 +2958,7 @@ package-menu--refresh
>            (when (and (package--has-keyword-p pkg keywords)
>                       (or package-list-unversioned
>                           (package--bi-desc-version (cdr elt)))
> -                     (or (eq packages t) (memq name packages)))
> +                     (or (not packages) (memq name packages)))
>              (push pkg info-list)))))
>  
>      ;; Available and disabled packages:
> @@ -2959,7 +2967,7 @@ package-menu--refresh
>      (dolist (elt package-archive-contents)
>        (let ((name (car elt)))
>          ;; To be displayed it must be in PACKAGES;
> -        (when (and (or (eq packages t) (memq name packages))
> +        (when (and (or (not packages) (memq name packages))

Does the above mean you are suggesting a backward-incompatible API
change?

> +Arguments PACKAGES and KEYWORDS are like `package-menu--refresh'."

Arguments cannot be "like" a function.  Suggest to say "like in
`package-menu--refresh'" instead.

I don't use package.el, so I'd like someone who does or knows the code
well to review the patch.

Thanks.