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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefan@marxist.se>
>> 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