[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#38424: [PATCH] Add new filter functions to Package Menu
From: |
Eli Zaretskii |
Subject: |
bug#38424: [PATCH] Add new filter functions to Package Menu |
Date: |
Sun, 01 Dec 2019 20:11:22 +0200 |
> 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?