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

[Stefan Kangas]

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.)

I meant to sent this earlier, but got too busy with real life.  I hope
that it's not too late to make it into Emacs 27.

Best regards,
Stefan Kangas

[Eli Zaretskii]

> From: Stefan Kangas <[hidden email]>
> 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.

[Stefan Kangas]

Eli Zaretskii <[hidden email]> writes:

>> From: Stefan Kangas <[hidden email]>
>> 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?

I don't know, but I'm also not sure I understand the benefit of not
adding them.

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.  Let me know if that's preferable.

>> * 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.

Is "Document the new commands." better?

>> -        (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.

>> +Arguments PACKAGES and KEYWORDS are like `package-menu--refresh'."
>
> Arguments cannot be "like" a function.  Suggest to say "like in
> `package-menu--refresh'" instead.

Right, I'll fix that.

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

I'll wait for that review.  Thank you for taking a look.

Best regards,
Stefan Kangas

[Stefan Kangas]

Stefan Kangas <[hidden email]> writes:

>>> * 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.
>
> Is "Document the new commands." better?

Wait, do you mean to also comment on moving the old commands around?
If so, I'd suggest the following: "Document the new commands and move
the fitering commands to follow a better sorting order."

Does that sound better?

Best regards,
Stefan Kangas

[Eli Zaretskii]

> From: Stefan Kangas <[hidden email]>
> Cc: [hidden email]
> 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?

[Eli Zaretskii]

> From: Stefan Kangas <[hidden email]>
> Cc: [hidden email]
> Date: Sun, 01 Dec 2019 12:16:43 +0100
>
> Stefan Kangas <[hidden email]> writes:
>
> >>> * 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.
> >
> > Is "Document the new commands." better?
>
> Wait, do you mean to also comment on moving the old commands around?

Yes.

> If so, I'd suggest the following: "Document the new commands and move
> the fitering commands to follow a better sorting order."

Much better, thanks.  Bonus points for saying what sorting order is
that, to make your motivation clear.