Re: [Elpa] Suggest new package: dired-quick-sort

[Hong Xu]

On 2025-01-29 Wed 11:56 GMT-08, Philip Kaludercic <philipk@posteo.net> wrote:

>
> Sorry for the delay in responding; I have a few comments here:

Thanks for the comment.

> -;; Copyright (C) 2016--2025 Hong Xu <hong@topbug.net>
> +;; Copyright (C) 2016-2025 Hong Xu <hong@topbug.net>

Is this an Emacs convention? -- is more like n-dash while - is hyphen.

> +;;     (require 'dired-quick-sort) ;<- are you assuming that the autoloads 
> are in effect or not?

Yes, I'm assuming autoloads are in effect.

>  (defun dired-quick-sort--format-switches ()
> -  "Return a dired-listing-switches string according to
> -`dired-quick-sort' settings."
> -  (format "%s %s %s %s %s" dired-listing-switches
> -          (if (string= dired-quick-sort-sort-by-last "default")
> -              ""
> -            (concat "--sort=" dired-quick-sort-sort-by-last))
> -          (if (char-equal dired-quick-sort-reverse-last ?y)
> -              "-r" "")
> -          (if (char-equal dired-quick-sort-group-directories-last ?y)
> -              "--group-directories-first" "")
> -          (if (not (string= dired-quick-sort-time-last "default"))
> -              (concat "--time=" dired-quick-sort-time-last) "")))
> -
> -(defun dired-quick-sort--sort-by-last (field)
> +  "Return a `dired-listing-switches' string according to `dired-quick-sort' 
> settings."
> +  (mapconcat
> +   #'identity                           ;perhaps `shell-quote-argument'?
> +   (list dired-listing-switches
> +         (if (string= dired-quick-sort-sort-by-last "default")
> +             ""                         ;`mapconcat' interprets nil as "", 
> which could simplify things here
> +           (concat "--sort=" dired-quick-sort-sort-by-last))
> +         (if (char-equal dired-quick-sort-reverse-last ?y) ;why not `eq'?
> +             "-r" "")
> +         (if (char-equal dired-quick-sort-group-directories-last ?y)
> +             "--group-directories-first" "")
> +         (if (not (string= dired-quick-sort-time-last "default"))
> +             (concat "--time=" dired-quick-sort-time-last) ""))
> +   " "))
> +
> +(defun dired-quick-sort--sort-by-last (field) ;please add a docstring!
>    (if (string= dired-quick-sort-sort-by-last field) "[X]" "[ ]"))
>  
> +;; you don't appear to have a hard dependency on hydra, right?  could you 
> also add a similar interface using transient?

I think it's possible, but I don't have the time to revamp this part
now. Is transient a hard requirement for adding to ELPA? If not, we can
do it as an improvement in the future.

>  `dired-quick-sort-suppress-setup-warning' to suppress warning and skip setup
>  silently.")
>      (if (not
> -         (with-temp-buffer
> +         (with-temp-buffer              ;this could also be checked at 
> compile-time, right?

Yes, but I decided to check at runtime because when Emacs starts up, the
environment may not be the same from last time. It can happen on a
*BSD/MacOS system where people can install the GNU Coreutils after
they've installed this package.

>> I'm also open to include this package in the GNU Elpa repository if this
>> is more preferred.
>
> That is fine, as long as all signficant contributors have signed the FSF
> copyright assignment.

How can I confirm if other signficant contributors have signed the
copyright assignment? I believe there are two other significant
contributors according to the git log, but I'm not sure whether one of
them have signed.

-- 
Hong