all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Justin Fields <justinlime1999@gmail.com>
Cc: emacs-devel@gnu.org
Subject: Re: [PATCH] Add file-ring to dired-aux.el
Date: Mon, 21 Oct 2024 12:48:11 +0300	[thread overview]
Message-ID: <86plnthjms.fsf@gnu.org> (raw)
In-Reply-To: <CAPafesGUrGkk2oaZgVgNtCr+d6WU9BOLzf9THhXuNFvS8TVf5A@mail.gmail.com> (message from Justin Fields on Mon, 21 Oct 2024 03:11:23 -0500)

> From: Justin Fields <justinlime1999@gmail.com>
> Date: Mon, 21 Oct 2024 03:11:23 -0500
> 
> From ca8f28008aeb95e74a0b5de28445cc25602ab724 Mon Sep 17 00:00:00 2001
> From: Justin Fields <justinlime1999@gmail.com>
> Date: Sun, 20 Oct 2024 22:54:57 -0500
> Subject: [PATCH] Dired-Aux: add file ring to dired

Thanks.  Some comments below.  If someone else has comments, please
post them.

> +;;; File ring
> +(defvar dired-file-ring nil
> +  "Captured dired files.")

Why do you call this a "ring"?  AFAICT, it is a simple list, created
and used as such.

> +(defun dired-file-ring-execute (action action-name file-list)
> +  "Execute a function to run for every item in the FILE-LIST.

The first line of a doc string should preferably mention all of the
mandatory arguments.

> +Argument ACTION argument to call with `dired-create-files' with as
> +its' FILE-CREATOR.

This reads awkwardly due to lack of punctuation.  Our usual style is
to say

  ACTION is passed to `dired-create-files' as its FILE-CREATOR
  argument.

Same comment for the other arguments.

> +Argument ACTION-NAME The name of the action, used for logging.

This should say "...for error logging by `dired-create-files'.", so
that readers could look up that logging.

> +  (dired-create-files action action-name file-list
> +    (lambda (file) (concat default-directory (file-name-nondirectory (directory-file-name file)))))

Why does this use 'concat' instead of 'expand-file-name'?

> +(defun dired-file-ring-capture ()
> +  "Capture marked Dired files to the file ring."
> +  (interactive)
> +  (mapc (lambda (file) (add-to-list 'dired-file-ring file)) (dired-get-marked-files))
> +  (message "Captured %s Files. %s files are present in the file ring" (length (dired-get-marked-files)) (length dired-file-ring)))

This will say "Captured 1 Files", which is incorrect English.  Please
use ngettext to do this better.

My suggestion is to rephrase this message to say something like

  %s files now in `file-ring', %s added

This makes the message much shorter and easier to understand, IMO.

> +(defun dired-file-ring-move ()
> +  "Move the file/s from the file ring to current dir.

By "current dir" do you mean default-directory?  We don't use the term
"current directory" in Emacs because Emacs pretends that each buffer
has its own "current directory".

> +This action clears the file ring."
> +  (interactive)
> +  (dired-file-ring-execute #'rename-file "MOVE" dired-file-ring)
> +  (dired-file-ring-clear))

What happens with files in subdirectories, e.g., if the user typed 'i'
before marking files?  Does this command create the corresponding
subdirectories in the default-directory when it moves the files?

> +(defun dired-file-ring-copy ()
> +  "Copy the file/s from the file ring to current dir."
> +  (interactive)
> +  (dired-file-ring-execute #'dired-copy-file "COPY" dired-file-ring))

Same question here.

> +(defun dired-file-ring-symlink ()
> +  "Create a symlink for the the file/s from the file ring to current dir."
> +  (interactive)
> +  (dired-file-ring-execute #'make-symbolic-link "SYM-LINK" dired-file-ring))
> +
> +(defun dired-file-ring-symlink-relative ()
> +  "Create a relative symlink for the the file/s from the file ring to current dir."
> +  (interactive)
> +  (dired-file-ring-execute #'dired-make-relative-symlink "RELATIVE SYM-LINK" dired-file-ring))

The doc strings of these two commands should be more explicit
regarding which file is the symlink and what will be its target.

This feature, when installed, will need a NEWS entry.

Finally, to accept a contribution of this size we need you to sign the
copyright assignment agreement.  Would you like to start this legal
paperwork rolling at this time?  If yes, I will send you the form to
fill and the instructions to go with it.



  reply	other threads:[~2024-10-21  9:48 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-21  8:11 [PATCH] Add file-ring to dired-aux.el Justin Fields
2024-10-21  9:48 ` Eli Zaretskii [this message]
2024-10-21 10:14   ` Justin Fields
2024-10-21 10:38     ` Eli Zaretskii
2024-10-28 10:31       ` Björn Bidar
     [not found]       ` <87v7xctt6p.fsf@>
2024-10-28 12:42         ` Eli Zaretskii
2024-10-21 12:35   ` Michael Heerdegen via Emacs development discussions.
2024-10-22 18:15 ` Juri Linkov
2024-10-22 18:43   ` Eli Zaretskii
2024-10-23  6:47     ` Justin Fields
2024-10-22 22:24   ` [External] : " Drew Adams
2024-10-26  7:02     ` Eshel Yaron
2024-10-27  7:47       ` Juri Linkov
2024-10-28  7:16         ` Eshel Yaron
2024-10-28 18:53           ` Juri Linkov
  -- strict thread matches above, loose matches on Subject: below --
2024-10-21 15:05 Justin Fields
2024-10-22 15:53 ` Michael Heerdegen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=86plnthjms.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=emacs-devel@gnu.org \
    --cc=justinlime1999@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.