Re: [ELPA] New package: dired-duplicates

[Harald Judt <h.judt@gmx.at>]

[-- Attachment #1.1: Type: text/plain, Size: 5246 bytes --]

Hi,


On 2023-10-31 13:21, Philip Kaludercic wrote:

[...]

> Sure, I can take care of adding the package to the archive, you'll only
> have to bump the "Version" header when you want the package to be
> released (and in the future as well, the commit that touches the
> "Version" header triggers a new release).
> 
> In the meantime, here is some quick and superficial feedback on the code:

Great news! Thank you for your feedback, I'll try to incorporate it and push 
the changes to the package git repository soon. Though I have a few questions 
understanding some of this:

> diff --git a/dired-duplicates.el b/dired-duplicates.el
> index d62af02..4af37a3 100644
> --- a/dired-duplicates.el
> +++ b/dired-duplicates.el
> @@ -54,7 +54,6 @@
>   (defcustom dired-duplicates-separate-results
>     t
>     "Boolean value indicating whether to separate results with new-lines."
> -  :group 'dired-duplicates

[...]

Why should I not use :group for customization? I thought this makes it easier 
to explore customization?

> @@ -103,21 +98,22 @@ return boolean t if the file matches a criteria, otherwise nil."
>   
>   The executable used is defined by `dired-duplicates-checksum-exec'."
>     (let* ((default-directory (file-name-directory (expand-file-name file)))
> -         (exec (executable-find dired-duplicates-checksum-exec t)))
> +         (exec (executable-find dired-duplicates-checksum-exec t))
> +	 (file (expand-file-name (file-local-name file))))
>       (unless exec
> -      (user-error "Checksum program %s not found in exec-path" exec))
> -    (car (split-string
> -          (shell-command-to-string
> -           (concat exec " \"" (expand-file-name (file-local-name file)) "\""))
> -          nil
> -          t))))
> +      (user-error "Checksum program %s not found in `exec-path'" exec))
> +    (with-temp-buffer
> +      (unless (zerop (call-process exec nil t nil file))
> +	(error "Failed to start checksum program %s" exec))
> +      (goto-char (point-min))
> +      (if (looking-at "\\`[[:alnum:]]+")
> +	  (match-string 0)
> +	(error "Unexpected output from checksum program %s" exec)))))

Yeah, good idea to improve the error handling.

[...]

>   (defun dired-duplicates--find-and-filter-files (directories)
>     "Search below DIRECTORIES for duplicate files.
> @@ -140,14 +136,14 @@ duplicate files as values."
>                       (append (gethash size same-size-table) (list f)))
>              finally
>              (cl-loop for same-size-files being the hash-values in same-size-table
> -                    if (> (length same-size-files) 1) do
> +                    if (> (length same-size-files) 1) do ;see length>
>                       (cl-loop for f in same-size-files
>                                for checksum = (dired-duplicates-checksum-file f)
>                                do (setf (gethash checksum checksum-table)
>                                         (append (gethash checksum checksum-table) (list f)))))
>              (cl-loop for same-files being the hash-value in checksum-table using (hash-key checksum)
>                       do
> -                    (if (> (length same-files) 1)
> +                    (if (cdr same-files) ;(> (length same-files) 1) in O(1)
>                           (setf (gethash checksum checksum-table)
>                                 (cons (file-attribute-size (file-attributes (car same-files)))
>                                       (sort same-files #'string<)))

Nice to know. Though I find length> more readable, I'll use cdr since I also 
use car ;-)

> @@ -180,7 +176,6 @@ Currently, this simply adds a new-line after each results group."
>                  for len in lengths
>                  do
>                  (forward-line len)
> -               ;; (forward-line len)
>                  (let ((inhibit-read-only t))
>                    (beginning-of-line)
>                    (unless (= (point) (point-max))

Thanks, I wonder how I have not noticed this.

> @@ -225,7 +220,7 @@ This is the same as `dired-do-flagged-delete', but calls
>   
>   (defvar dired-duplicates-map
>     (let ((map (make-sparse-keymap)))
> -    ;; workaround for Emacs bug #57565
> +    ;; workaround for Emacs bug#57565
>       (when (< emacs-major-version 29)
>         (define-key map (kbd "x") 'dired-duplicates--do-flagged-delete)
>         (define-key map (kbd "D") 'dired-duplicates--do-delete))
> @@ -248,7 +243,7 @@ The results will be shown in a Dired buffer."
>                                                  default-directory)))
>     (unless directories
>       (user-error "Specify one or more directories to search in"))
> -  (let* ((directories (if (listp directories) directories (list directories)))
> +  (let* ((directories (if (listp directories) directories (list directories))) ;see `ensure-list'

`ensure-list' would bump emacs requirements to 28.1 or require compat hence I 
did not use it.

[...]

Thanks for the review and your suggestions.

Regards,
Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]