GNU bug report logs - #47957
dired-aux.el: convert comments to doctrings

Previous Next

Package: emacs;

Reported by: Boruch Baum <boruch_baum <at> gmx.com>

Date: Thu, 22 Apr 2021 19:46:02 UTC

Severity: minor

Tags: wontfix

Done: Lars Ingebrigtsen <larsi <at> gnus.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 47957 in the body.
You can then email your comments to 47957 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Thu, 22 Apr 2021 19:46:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Boruch Baum <boruch_baum <at> gmx.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 22 Apr 2021 19:46:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Emacs Bug Reporting <bug-gnu-emacs <at> gnu.org>
Subject: diredx-aux doctrings [PATCH INCLUDED]
Date: Thu, 22 Apr 2021 15:45:08 -0400
[Message part 1 (text/plain, inline)]
File dired-aux.el had many docstrings in comment form, meaning that they
were non discoverable using `describe-function'. The attached patch
fixes that.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0
[dired-aux.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Thu, 22 Apr 2021 20:17:01 GMT) Full text and rfc822 format available.

Message #8 received at 47957 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Boruch Baum <boruch_baum <at> gmx.com>, "47957 <at> debbugs.gnu.org"
 <47957 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#47957: diredx-aux doctrings [PATCH INCLUDED]
Date: Thu, 22 Apr 2021 20:15:56 +0000
Thanks for doing this.

A minor suggestion (for consideration) for
`dired-do-kill-lines', based on my feeling
that "kill" here is a misleading misnomer:

Say "remove", not "kill" (which generally
has to do with removing text and placing it
on the kill ring).

FWIW, I have this doc string for it in
Dired+.  Apart from the added optional arg
INIT-COUNT, the behavior is the same.
___

Remove all marked lines, or the next ARG lines.
The files or directories on those lines are _not_ deleted.  Only the
Dired listing is affected.  To restore the removals, use `M-x revert-buffer'.

With a numeric prefix arg, remove that many lines going forward,
starting with the current line.  (A negative prefix arg removes lines
going backward.)

If you use a prefix arg to remove the line for a subdir whose listing
you have inserted into the Dired buffer, then that subdir listing is
also removed.

To remove a subdir listing _without_ removing the subdir's line in its
parent listing, go to the header line of the subdir listing and use
this command with any prefix arg.

When called from Lisp, non-nil INIT-COUNT is added to the number of
lines removed by this invocation, for the reporting message.
___

(Also, don't forget to prefix lines that start with ( with \.)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Sun, 25 Apr 2021 05:23:01 GMT) Full text and rfc822 format available.

Message #11 received at 47957 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: "47957 <at> debbugs.gnu.org" <47957 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#47957: diredx-aux doctrings [PATCH INCLUDED]
Date: Sun, 25 Apr 2021 01:22:12 -0400
[Message part 1 (text/plain, inline)]
On 2021-04-22 20:15, Drew Adams wrote:
> Thanks for doing this.

Quite welcome. Let's see what it takes to get merged...

> A minor suggestion (for consideration) for `dired-do-kill-lines'
> ...
> Say "remove", not "kill" (which generally has to do with removing
> ...
> (Also, don't forget to prefix lines that start with ( with \.)

Done. Updated patch attached.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0
[dired-aux-2.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Sun, 25 Apr 2021 08:16:02 GMT) Full text and rfc822 format available.

Message #14 received at 47957 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: drew.adams <at> oracle.com, 47957 <at> debbugs.gnu.org
Subject: Re: bug#47957: [External] : bug#47957: diredx-aux doctrings [PATCH
 INCLUDED]
Date: Sun, 25 Apr 2021 11:15:23 +0300
> Date: Sun, 25 Apr 2021 01:22:12 -0400
> From: Boruch Baum <boruch_baum <at> gmx.com>
> Cc: "47957 <at> debbugs.gnu.org" <47957 <at> debbugs.gnu.org>
> 
> Done. Updated patch attached.

Thanks.  This still needs some work to make it compatible with our
conventions.  First, some general issues will almost all of the doc
strings in the patch:

  . The first line of a doc string should be a complete sentence
    mentioning the mandatory arguments.
  . Two spaces between sentences, per US English conventions.
  . An opening parenthesis or grave accent in column zero should be
    escaped by a backslash.

A few specific comments:

>  (defun dired-map-dired-file-lines (fun)
> -  ;; Perform FUN with point at the end of each non-directory line.
> -  ;; FUN takes one argument, the absolute filename.
> +"Perform FUN with point at the end of each non-directory line.

"Perform FUN" sounds awkward.  I suggest "Call FUN" instead.

>  (defun dired-compress ()
> -  ;; Compress or uncompress the current file.
> -  ;; Return nil for success, offending filename else.

This and other uses of "offending" are not really a good idea.  We are
not talking about files that cause some trouble.  Suggest to find a
better word.

> -;; Read arguments for a marked-files command that wants a file name,
> -;; perhaps popping up the list of marked files.
> -;; ARG is the prefix arg and indicates whether the files came from
> -;; marks (ARG=nil) or a repeat factor (integerp ARG).
> -;; If the current file was used, the list has but one element and ARG
> -;; does not matter. (It is non-nil, non-integer in that case, namely '(4)).
> -;; DEFAULT is the default value to return if the user just hits RET;
> -;; if it is omitted or nil, then the name of the directory is used.
> -
>  (defun dired-mark-read-file-name (prompt dir op-symbol arg files
>  					 &optional default)
> +  "Read arguments for a marked-files command that wants a file name,
> +perhaps popping up the list of marked files. ARG is the prefix
> +arg and indicates whether the files came from marks (ARG=nil) or
> +a repeat factor (integerp ARG). If the current file was used, the
> +list has but one element and ARG does not matter. (It is non-nil,
> +non-integer in that case, namely '(4)). DEFAULT is the default
> +value to return if the user just hits RET; if it is omitted or
> +nil, then the name of the directory is used."

Some of the comments that you converted to doc strings are
descriptions of the implementation.  the above is one example.  I'm
not sure it is a good idea to have doc strings which do that, because
understanding how to use this function in Lisp programs is not easy
given such low-level details and nothing else.  Maybe just leave them
as comments?

> +  "Return a list of default values for file-reading functions in Dired.
> +This list may contain directories from Dired buffers in other windows.
> +`fn-list' is a list of file names used to build a list of defaults.
> +When nil or more than one element, a list of defaults will
> +contain only directory names.  `target-dir' is a directory name

This uses non-standard way of naming the function's arguments: they
should be up-cased and not quoted.

> +FILE-CREATOR and OPERATION as in dired-create-files.
> +ARG as in dired-get-marked-files.

These two sentences lack the verb ("are", I guess).

>  (defun dired-alist-sort ()
> -  ;; Keep the alist sorted on buffer position.
> +  "Keep the alist sorted on buffer position."

This references some alist without naming it.

>  (defun dired-insert-subdir-newpos (new-dir)
> -  ;; Find pos for new subdir, according to tree order.
> +  "Find pos for new subdir, according to tree order."

This doesn't mention the function's argument.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Sun, 25 Apr 2021 09:13:01 GMT) Full text and rfc822 format available.

Message #17 received at 47957 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: drew.adams <at> oracle.com, 47957 <at> debbugs.gnu.org
Subject: Re: bug#47957: [External] : bug#47957: diredx-aux doctrings [PATCH
 INCLUDED]
Date: Sun, 25 Apr 2021 05:12:15 -0400
On 2021-04-25 11:15, Eli Zaretskii wrote:
> > Date: Sun, 25 Apr 2021 01:22:12 -0400
> > From: Boruch Baum <boruch_baum <at> gmx.com>
> > Cc: "47957 <at> debbugs.gnu.org" <47957 <at> debbugs.gnu.org>
> >
> > Done. Updated patch attached.
>
> Thanks.  This still needs some work to make it compatible with our
> conventions.

All your feedback is well-taken, but I'm moving on to other things with
the patch as submitted. If as an improvement over the existing file the
patch isn't good enough to be committed for someone to continue with,
so be it.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0




Removed tag(s) patch. Request was from Stefan Kangas <stefan <at> marxist.se> to control <at> debbugs.gnu.org. (Sun, 25 Apr 2021 11:03:02 GMT) Full text and rfc822 format available.

Changed bug title to 'dired-aux.el: convert comments to doctrings' from 'diredx-aux doctrings [PATCH INCLUDED]' Request was from Stefan Kangas <stefan <at> marxist.se> to control <at> debbugs.gnu.org. (Sun, 25 Apr 2021 11:03:02 GMT) Full text and rfc822 format available.

Severity set to 'minor' from 'normal' Request was from Stefan Kangas <stefan <at> marxist.se> to control <at> debbugs.gnu.org. (Sun, 25 Apr 2021 11:03:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Sun, 25 Apr 2021 18:22:02 GMT) Full text and rfc822 format available.

Message #26 received at 47957 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: "47957 <at> debbugs.gnu.org" <47957 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#47957: diredx-aux doctrings [PATCH INCLUDED]
Date: Sun, 25 Apr 2021 18:21:04 +0000
> > Thanks for doing this.
> 
> Quite welcome. Let's see what it takes to get merged...
> 
> > A minor suggestion (for consideration) for `dired-do-kill-lines'
> > ...
> > Say "remove", not "kill" (which generally has to do with removing
> > ...
> > (Also, don't forget to prefix lines that start with ( with \.)
> 
> Done. Updated patch attached.

Some feedback, for consideration.  I see now, after writing
all this, that Eli also provided feedback.  HTH, anyway.

1.
+Operates on marked files and refresh their file lines.
                              refreshes

Start each <ARG NAME> is... on its own line, including
OP-SYMBOL and ARG.

2.
+"Process all the files in FILES...

  Process FILES...

+Uses (FUNCALL FUNCTION ARGS... SOME-FILES...).

 Applies FUNCTION to ARGS and SOME-FILES, where SOME-FILES
 is a batch of at most MAX files.

(The comment in the code was wrong.)

3.
+"Make up a shell command line from 

  Return a shell command line derived from

+If ON-EACH is t, COMMAND should be applied to each file, else
+simply concat all files and apply COMMAND to this.
+FILE-LIST's elements will be quoted for the shell.

 By default, apply COMMAND to all files together.
 Non-nil ON-EACH means apply COMMAND to each file instead.

 Quote the files in FILE-LIST for the shell.

+Might be redefined for smarter things and could then use RAW-ARG
+(coming from interactive P and currently ignored) to decide what to do.
+Smart would be a way to access basename or extension of file names."

Remove that - I think it should just remain a code comment.

4.
You indicate that you applied my suggestion to speak of
removing, rather than killing, lines.  But I don't see
that you did that, at all.

5.
+"Compress or uncompress the current file.
+Return nil for success, offending filename else."

  Compress or uncompress the file named on the current line.
  Return nil for success or the file name for failure.

6.
+"Request confirmation from the user for an operation on marked-files.
+The operation is described by OP-SYMBOL. Confirmation consists in
+a y-or-n question with a file list pop-up unless OP-SYMBOL is a
+member of `dired-no-confirm'.

  Request confirmation from user for an operation on the marked files.
  OP-SYMBOL is a symbol describing the operation performed (e.g.
  `compress').

  If `dired-no-confirm' is t or OP-SYMBOL is a member of list
  `dired-no-confirm' then this is a no-op: no confirmation is needed.
  Otherwise, confirmation is requested with `y-or-n-p'.

7.
FWIW: For `dired-map-over-marks-check' I use this (adapted
by removing mention of added &rest arg FUN-ARGS and function
`diredp-map-over-marks-and-report'):

Map FUN over marked lines and report failures.
FUN should return nil for success and non-nil (the offending object,
e.g. the short form of the filename) for a failure.  FUN can log a
detailed error explanation using `dired-log'.

MARK-ARG is as the second argument of `dired-map-over-marks'.

OP-SYMBOL is a symbol describing the operation performed (e.g.
`compress').  It is used with `dired-mark-pop-up' to prompt the user
\(e.g. with `Compress * [2 files]? ') and to display errors (e.g.
`Failed to compress 1 of 2 files - type ? for details (\"foo\")')

SHOW-PROGRESS if non-nil means redisplay Dired after each file.

FUN-ARGS is the list of any remaining args to
`dired-map-over-marks-check'.  Function FUN is applied to these
arguments.

8.
+  "Byte-compile file at POINT.

and

+  "Load file at POINT.

POINT shouldn't be uppercase.  And it shouldn't be mentioned.
The cursor need not be on the file name.

Use something like this instead:

   Byte-compile the file named on the current line.

9.
+If FILE is nil, then just delete the current line. Keeps any
+marks that may be present in column one (doing this here is
+faster than with dired-add-entry's optional arg). Does not update
+other dired buffers. Use dired-relist-entry for that."

 If FILE is nil, then just delete the current line.
 Keep any mark in column one.

 This does not update other dired buffers.
 \(Use `dired-relist-entry' for that.)

10.
+  "Return pos of first file line of DIR.
    Return the position of the first file line of DIR.
    DIR is assumed to be visible, not hidden.

+Header lines and total or wildcard lines are skipped. Important:
+never moves into the next subdir. DIR is assumed to be unhidden."

    This never moves into the next subdir listing.

(No need to say what's skipped, as it says "first file line".)

11.
+  "A fluid var used in dired-handle-overwrite.
+It should be let-bound whenever dired-copy-file etc are called.
+See `dired-create-files' for an example.")

I don't know what this is - it was apparently added after Emacs 27.
But the first line should not say what it says.  The first line
should say what the variable does, is, or represents.  Presumably
its value controls or defines how overwriting is handled.

This doc string is not good, IMO.  Maybe the variable itself
is ill-advised; dunno.  Maybe that info is helpful as a code
comment; dunno.

12.
What happened to a doc string for `dired-rename-subdir'?  That's
more important than doc strings for `*-1' and `*-2'.  The latter
doc strings should say these are helper functions for the main fn.

13.
+"Rename DIR to TO in header lines and dired-subdir-alist, if DIR
+or one of its subdirectories is expanded in this buffer."

The args should have the same names as in `dired-rename-subdir'
(so a code change also).

  Helper for `dired-rename-subdir'.
  Rename only if FROM-DIR or one of its subdirectories is...

(Requires study of the code.  Sorry, no time.)

14.
+  "Update the headerline and dired-subdir-alist element, as well
+as dired-switches-alist element, of directory described by
+alist-element ELT to reflect the moving of DIR to TO. Thus, ELT
+describes either DIR itself or a subdir of DIR."

Start with "Helper for `dired-rename-subdir-1'."

headerline -> header line

   ELT is an element of `dired-subdir-alist'.  It describes
   either FROM-DIR or one of its subdirectories.

15.
+  "Read arguments for a marked-files command that wants a file name,
+perhaps popping up the list of marked files. ARG is the prefix
+arg and indicates whether the files came from marks (ARG=nil) or
+a repeat factor (integerp ARG). If the current file was used, the
+list has but one element and ARG does not matter. (It is non-nil,
+non-integer in that case, namely '(4)). DEFAULT is the default
+value to return if the user just hits RET; if it is omitted or
+nil, then the name of the directory is used."

    Use `dired-mark-pop-up' to read a file name.
    PROMPT, ARG, and FILES are used for prompting.
     PROMPT is a `format' string that is applied to the result of applying
     `dired-mark-prompt' to ARG and FILES.
    OP-SYMBOL, FILES, DIR, and DEFAULT are passed to `dired-mark-pop-up'.

16.
+  "Return directories from all next visible windows with dired-mode buffers."
    Return a list of directories from visible `next-window' Dired buffers.
    
17.
+  "Return directories from all visible windows with dired-mode
+buffers ordered by most-recently-used."

    Return a list of directories from visible recent Dired buffers.

18.
+  "Try to guess which target directory the user may want.
+If there is a dired buffer displayed in one of the next windows,
+use its current subdir, else use current subdir of this dired buffer."

    Return a directory the user might want as a target.
    Prefer the current subdir of a Dired `next-window' buffer.
    If none, return the current subdir of this buffer.

19.
+  "Return a list of default values for file-reading functions in Dired.
+This list may contain directories from Dired buffers in other windows.
+`fn-list' is a list of file names used to build a list of defaults.
+When nil or more than one element, a list of defaults will
+contain only directory names.  `target-dir' is a directory name
+to exclude from the returned list, for the case when this
+directory name is already presented in initial input.
+For Dired operations that support `dired-dwim-target',
+the argument `target-dir' should have the value returned
+from `dired-dwim-target-directory'."

Change arg FN-LIST to FILES.  Then it can be used in the doc
with no other description.

   Return a list of default values for file-reading functions.
   This can include directories from Dired buffers in other windows.

   If FILES is a singleton list (FILE) then the return value can
    include this or other directories with FILE appended, as well
    as just other directories.
   If FILES is not a singleton list then the return value includes
    only directories.

Sorry, but I've run out of time to spend on this.  For #19,
TARGET-DIR needs to be described properly.  The comment doesn't
do that, AFAICT.

HTH.  Can't spend more time on this.  Hopefully someone else can
take a look.  It's not enough to move the comments to doc strings,
I think.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Sun, 25 Apr 2021 19:39:02 GMT) Full text and rfc822 format available.

Message #29 received at 47957 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: "47957 <at> debbugs.gnu.org" <47957 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#47957: diredx-aux doctrings [PATCH INCLUDED]
Date: Sun, 25 Apr 2021 15:38:41 -0400
[Message part 1 (text/plain, inline)]
On 2021-04-25 18:21, Drew Adams wrote:
> You indicate that you applied my suggestion to speak of
> removing, rather than killing, lines.  But I don't see
> that you did that, at all.

Oops. Sent the wrong diff, I guess. Sorry. Here it is now.

> Sorry, but I've run out of time to spend on this.

Me too. At least for now.

> HTH.  Can't spend more time on this.  Hopefully someone else can
> take a look.  It's not enough to move the comments to doc strings,
> I think.

It was an easy lift to offer a big improvement, but agreed, not perfect.
My opinion is that until the work can be continued, it's desirable over
the prior state, but that's not for me to decide.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0
[dired-aux-3.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#47957; Package emacs. (Mon, 03 May 2021 08:29:01 GMT) Full text and rfc822 format available.

Message #32 received at 47957 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, drew.adams <at> oracle.com, 47957 <at> debbugs.gnu.org
Subject: Re: bug#47957: dired-aux.el: convert comments to doctrings
Date: Mon, 03 May 2021 10:27:57 +0200
Boruch Baum <boruch_baum <at> gmx.com> writes:

> All your feedback is well-taken, but I'm moving on to other things with
> the patch as submitted. If as an improvement over the existing file the
> patch isn't good enough to be committed for someone to continue with,
> so be it.

Looking over the changes -- some look good to me, and some don't.
Converting comments to doc strings isn't a mechanical process --
comments should describe what a function does, while internal details
should be left as comments.

So I'm closing this bug report without applying the patch, because in
total I think it's a change for the negative.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




Added tag(s) wontfix. Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Mon, 03 May 2021 08:29:02 GMT) Full text and rfc822 format available.

bug closed, send any further explanations to 47957 <at> debbugs.gnu.org and Boruch Baum <boruch_baum <at> gmx.com> Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Mon, 03 May 2021 08:29:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Mon, 31 May 2021 11:24:07 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 303 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.