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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Cc: 38424@debbugs.gnu.org
> Date: Sun, 01 Dec 2019 12:12:58 +0100
> 
> > 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?
> 
> I don't know, but I'm also not sure I understand the benefit of not
> adding them.

The same reason why we don't have string-greater-p: keep Emacs from
becoming larger without a good reason.

> In this case, it let me do this:
> 
> +       (let ((fun (cond ((eq predicate '=) 'version-list-=)
> +                        ((eq predicate '<) 'version-list-<)
> +                        ((eq predicate '>) 'version-list->)
> 
> I could of course use a lambda here instead, but it makes the code a
> bit uglier.

Or you could use a defsubst or an inline function.

> >> * 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.)
> 
> Thanks.  I thought that was how we usually wrote.

We do, but in a different situation.  Like this:

 * lisp/foo.el (foo-bar-quux-func): New function.
 * doc/lispref/foo-docs.texi (Foo Bar): Document it.

In this case, it's immediately clear what "it" refers to.  But in your
case:

 * lisp/emacs-lisp/package.el (package-menu-filter-by-version)
 (package-menu-filter-by-status, package-menu-filter-by-archive):
 New filter functions.
 (package-menu--filter-by): New helper function.
 (package-menu-filter-by-keyword, package-menu-filter-by-name): Use
 above helper function.
 (package-menu-mode-menu):
 (package-menu-mode-map): Update menu to include new filter functions.
 * doc/emacs/package.texi (Package Menu): Document it.
 * etc/NEWS: Announce it.

it is much less clear, because there are many "its" above.

> >> -        (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?
> 
> AFAIU, this does not change the API since this is an internal
> function.

But doesn't it change the API of that internal function in
incompatible ways?  If it does, was that really necessary?