Small docstring improvements

Small docstring improvements

[Philipp Stephani]

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

Hi,

here's a patch that clarifies some docstrings a bit. Please take a look.
I'll push to master if this is OK.

Thanks,
Phil

[-- Attachment #1.2: Type: text/html, Size: 214 bytes --]

[-- Attachment #2: 0001-Some-assorted-documentation-clarifications.patch --]
[-- Type: application/octet-stream, Size: 2671 bytes --]

From 75c2067eca12a784c57286c751b17cbf1bfda47e Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Fri, 19 Aug 2016 21:23:24 +0200
Subject: [PATCH] Some assorted documentation clarifications.

* src/fileio.c (Fwrite_region): Clarify that END is ignored if
START is nil.
* src/editfns.c (Fbuffer_size): Add short discussion about
narrowing.
* src/callproc.c (Fcall_process_region): Discuss behavior when
START and END are not buffer positions.
---
 src/callproc.c | 7 +++++++
 src/editfns.c  | 7 ++++++-
 src/fileio.c   | 3 ++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index 487115d..454ed6d 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1004,6 +1004,13 @@ create_temp_file (ptrdiff_t nargs, Lisp_Object *args,
 DEFUN ("call-process-region", Fcall_process_region, Scall_process_region,
        3, MANY, 0,
        doc: /* Send text from START to END to a synchronous process running PROGRAM.
+
+START and END are normally buffer positions specifying the part of the
+buffer to send to the process.
+If START is nil, that means to use the entire buffer contents; END is
+ignored.
+If START is a string, then send that string to the process
+instead of any buffer contents; END is ignored.
 The remaining arguments are optional.
 Delete the text if fourth arg DELETE is non-nil.
 
diff --git a/src/editfns.c b/src/editfns.c
index 732306a..4efb76b 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -1065,7 +1065,12 @@ usage: (save-current-buffer &rest BODY)  */)
 \f
 DEFUN ("buffer-size", Fbuffer_size, Sbuffer_size, 0, 1, 0,
        doc: /* Return the number of characters in the current buffer.
-If BUFFER, return the number of characters in that buffer instead.  */)
+If BUFFER is not nil, return the number of characters in that buffer
+instead.
+
+This does not take narrowing into account; to count the number of
+characters in the accessible portion of the buffer, use
+‘(- (point-max) (point-min))’.  */)
   (Lisp_Object buffer)
 {
   if (NILP (buffer))
diff --git a/src/fileio.c b/src/fileio.c
index 37948bc..bf6bf62 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4654,7 +4654,8 @@ DEFUN ("write-region", Fwrite_region, Swrite_region, 3, 7,
 When called from a program, requires three arguments:
 START, END and FILENAME.  START and END are normally buffer positions
 specifying the part of the buffer to write.
-If START is nil, that means to use the entire buffer contents.
+If START is nil, that means to use the entire buffer contents; END is
+ignored.
 If START is a string, then output that string to the file
 instead of any buffer contents; END is ignored.
 
-- 
2.9.0

Re: Small docstring improvements

[John Wiegley]

>>>>> "PS" == Philipp Stephani <p.stephani2@gmail.com> writes:

PS> +START and END are normally buffer positions specifying the part of the
PS> +buffer to send to the process.
PS> +If START is nil, that means to use the entire buffer contents; END is
PS> +ignored.

I would phrase a little differently:

    +If START and END are both buffer positions, they specify the region
    +to send to the process.
    +If START is nil, it means to use the entire buffer contents; END is
    +ignored.

PS> +If START is a string, then send that string to the process +instead of
PS> any buffer contents; END is ignored.

This is a horrible abuse. There should be another function
`call-process-with-string'.

I don't like the way that "START" has three separate meanings, only one of
which has anything to do with beginning a START of something.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Small docstring improvements

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

John Wiegley <jwiegley@gmail.com> schrieb am Fr., 19. Aug. 2016 um
21:34 Uhr:

> >>>>> "PS" == Philipp Stephani <p.stephani2@gmail.com> writes:
>
> PS> +START and END are normally buffer positions specifying the part of the
> PS> +buffer to send to the process.
> PS> +If START is nil, that means to use the entire buffer contents; END is
> PS> +ignored.
>
> I would phrase a little differently:
>
>     +If START and END are both buffer positions, they specify the region
>     +to send to the process.
>     +If START is nil, it means to use the entire buffer contents; END is
>     +ignored.
>
> PS> +If START is a string, then send that string to the process +instead of
> PS> any buffer contents; END is ignored.
>
> This is a horrible abuse. There should be another function
> `call-process-with-string'.
>
> I don't like the way that "START" has three separate meanings, only one of
> which has anything to do with beginning a START of something.
>
>
I've simply copied the docstring from write-region, which has exactly the
same semantics (because call-process-region calls write-region). Like it or
not, these are the semantics, and they are intentional (the code for
call-process-region has explicit branches for these cases), so I think they
deserve to be documented.

[-- Attachment #2: Type: text/html, Size: 1763 bytes --]

Re: Small docstring improvements

[John Wiegley]

>>>>> "PS" == Philipp Stephani <p.stephani2@gmail.com> writes:

PS> I've simply copied the docstring from write-region, which has exactly the
PS> same semantics (because call-process-region calls write-region). Like it
PS> or not, these are the semantics, and they are intentional (the code for
PS> call-process-region has explicit branches for these cases), so I think
PS> they deserve to be documented.

Yes, agreed with respect to your patch, disagree only with respect to what you
found there. :) Those explicit branches shouldn't have been written. It sounds
like its purely an attempt to avoid creating new functions for what is clearly
new functionality.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Small docstring improvements

[Dmitry Gutov]

On 08/19/2016 10:26 PM, Philipp Stephani wrote:
> +This does not take narrowing into account; to count the number of
> +characters in the accessible portion of the buffer, use
> +тАШ(- (point-max) (point-min))тАЩ.  */)

We don't use curly quotes as markup in Elisp docstrings. Please use ` and '.

RE: Small docstring improvements

[Drew Adams]

> Yes, agreed with respect to your patch, disagree only with respect to what
> you found there. :) Those explicit branches shouldn't have been written. It
> sounds like its purely an attempt to avoid creating new functions for what is
> clearly new functionality.

FWIW, I disagree.  I have no problem with a parameter having
different uses and different types (buffer position, string, nil).

It could be argued that the parameter name does not reflect all
of the use cases, or that it should have a generic name, such as
ARG, because of the multiple uses.  But a buffer-position is the
common use case, and I don't have a problem with the name "START",
as long as the doc clearly specifies the full behavior.

FWIW2, it is not a new feature.  From the Emacs 20 doc string for
`write-region' - but the writer seems to agree with you that this
(handy) feature is a kludge:

  Kludgy feature: if START is a string, then that string is written
  to the file, instead of any buffer contents, and END is ignored.

The use of a `nil' value for START was added in Emacs 22.  At the
same time, the characterization of multiple-use START as a kludge
was removed.

Re: Small docstring improvements

[John Wiegley]

>>>>> Drew Adams <drew.adams@oracle.com> writes:

> FWIW, I disagree.  I have no problem with a parameter having
> different uses and different types (buffer position, string, nil).

We agree to disagree. Semantic overloading like this I consider anathema.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Small docstring improvements

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 19 Aug 2016 19:26:14 +0000
> 
> here's a patch that clarifies some docstrings a bit. Please take a look. I'll push to master if this is OK.

Some comments below.  Please push to emacs-25, not to master, as this
is just a documentation change, and there's no reason to delay it
until after 25.1.

> +If START is nil, that means to use the entire buffer contents; END is
> +ignored.

"the entire accessible portion of the buffer" is more accurate than
"the entire buffer contents", when narrowing is in effect.

> --- a/src/editfns.c
> +++ b/src/editfns.c
> @@ -1065,7 +1065,12 @@ usage: (save-current-buffer &rest BODY)  */)
>  \f
>  DEFUN ("buffer-size", Fbuffer_size, Sbuffer_size, 0, 1, 0,
>         doc: /* Return the number of characters in the current buffer.
> -If BUFFER, return the number of characters in that buffer instead.  */)
> +If BUFFER is not nil, return the number of characters in that buffer
> +instead.
> +
> +This does not take narrowing into account; to count the number of
> +characters in the accessible portion of the buffer, use
> +‘(- (point-max) (point-min))’.  */)

The advice in the last sentence doesn't work when BUFFER is non-nil.
(I actually doubt that the advice is needed here; it's enough to have
something like that in the ELisp manual -- which already hints on
that, but maybe we should make that more explicit.

> @@ -4654,7 +4654,8 @@ DEFUN ("write-region", Fwrite_region, Swrite_region, 3, 7,
>  When called from a program, requires three arguments:
>  START, END and FILENAME.  START and END are normally buffer positions
>  specifying the part of the buffer to write.
> -If START is nil, that means to use the entire buffer contents.
> +If START is nil, that means to use the entire buffer contents; END is
> +ignored.

Once again, please mention the accessible portion of the buffer.

Thanks.

Re: Small docstring improvements

[Eli Zaretskii]

> From: John Wiegley <jwiegley@gmail.com>
> Date: Fri, 19 Aug 2016 12:34:40 -0700
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> PS> +If START is a string, then send that string to the process +instead of
> PS> any buffer contents; END is ignored.
> 
> This is a horrible abuse. There should be another function
> `call-process-with-string'.

I disagree with you about the abuse: it sounds polymorphic to me.  But
maybe that's because I'm too used to this paradigm.

In any case, that ship has sailed long ago: we have this feature in
this and other functions for as long as I can remember, and they are
used in quite a few places.

> I don't like the way that "START" has three separate meanings, only one of
> which has anything to do with beginning a START of something.

Once again, I don't see why you dislike that.  Lisp is not C.

Re: Small docstring improvements

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 20. Aug. 2016 um 09:08 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Fri, 19 Aug 2016 19:26:14 +0000
> >
> > here's a patch that clarifies some docstrings a bit. Please take a look.
> I'll push to master if this is OK.
>
> Some comments below.  Please push to emacs-25, not to master, as this
> is just a documentation change, and there's no reason to delay it
> until after 25.1.
>

Thanks.


>
> > +If START is nil, that means to use the entire buffer contents; END is
> > +ignored.
>
> "the entire accessible portion of the buffer" is more accurate than
> "the entire buffer contents", when narrowing is in effect.
>

That would be incorrect, as these functions ignore narrowing.


>
> > --- a/src/editfns.c
> > +++ b/src/editfns.c
> > @@ -1065,7 +1065,12 @@ usage: (save-current-buffer &rest BODY)  */)
> >
> >  DEFUN ("buffer-size", Fbuffer_size, Sbuffer_size, 0, 1, 0,
> >         doc: /* Return the number of characters in the current buffer.
> > -If BUFFER, return the number of characters in that buffer instead.  */)
> > +If BUFFER is not nil, return the number of characters in that buffer
> > +instead.
> > +
> > +This does not take narrowing into account; to count the number of
> > +characters in the accessible portion of the buffer, use
> > +‘(- (point-max) (point-min))’.  */)
>
> The advice in the last sentence doesn't work when BUFFER is non-nil.
> (I actually doubt that the advice is needed here; it's enough to have
> something like that in the ELisp manual -- which already hints on
> that, but maybe we should make that more explicit.
>

I'd prefer a bit of redundancy in a very basic function such as this. At
least I wouldn't want to search the Elisp manual for additional information
unless really necessary. I'd reword it to "the accessible portion of the
current buffer".


>
> > @@ -4654,7 +4654,8 @@ DEFUN ("write-region", Fwrite_region,
> Swrite_region, 3, 7,
> >  When called from a program, requires three arguments:
> >  START, END and FILENAME.  START and END are normally buffer positions
> >  specifying the part of the buffer to write.
> > -If START is nil, that means to use the entire buffer contents.
> > +If START is nil, that means to use the entire buffer contents; END is
> > +ignored.
>
> Once again, please mention the accessible portion of the buffer.
>
>
See above, this function ignores narrowing.

[-- Attachment #2: Type: text/html, Size: 3507 bytes --]

Re: Small docstring improvements

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 20 Aug 2016 13:52:03 +0000
> Cc: emacs-devel@gnu.org
> 
>  > +If START is nil, that means to use the entire buffer contents; END is
>  > +ignored.
> 
>  "the entire accessible portion of the buffer" is more accurate than
>  "the entire buffer contents", when narrowing is in effect.
> 
> That would be incorrect, as these functions ignore narrowing.

You are right, I didn't see that write_region calls widen.  Sorry.

>  > +This does not take narrowing into account; to count the number of
>  > +characters in the accessible portion of the buffer, use
>  > +‘(- (point-max) (point-min))’. */)
> 
>  The advice in the last sentence doesn't work when BUFFER is non-nil.
>  (I actually doubt that the advice is needed here; it's enough to have
>  something like that in the ELisp manual -- which already hints on
>  that, but maybe we should make that more explicit.
> 
> I'd prefer a bit of redundancy in a very basic function such as this. At least I wouldn't want to search the Elisp
> manual for additional information unless really necessary. I'd reword it to "the accessible portion of the current
> buffer".

If we want to give advice like that, IMO we should also give such an
advice for when BUFFER is non-nil.

Re: Small docstring improvements

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 20. Aug. 2016 um 17:34 Uhr:

> >  > +This does not take narrowing into account; to count the number of
> >  > +characters in the accessible portion of the buffer, use
> >  > +‘(- (point-max) (point-min))’. */)
> >
> >  The advice in the last sentence doesn't work when BUFFER is non-nil.
> >  (I actually doubt that the advice is needed here; it's enough to have
> >  something like that in the ELisp manual -- which already hints on
> >  that, but maybe we should make that more explicit.
> >
> > I'd prefer a bit of redundancy in a very basic function such as this. At
> least I wouldn't want to search the Elisp
> > manual for additional information unless really necessary. I'd reword it
> to "the accessible portion of the current
> > buffer".
>
> If we want to give advice like that, IMO we should also give such an
> advice for when BUFFER is non-nil.
>

OK, then I'd append ", and to count the number of characters
in some other BUFFER, use
`(with-current-buffer BUFFER (- (point-max) (point-min)))'.".

[-- Attachment #2: Type: text/html, Size: 1485 bytes --]

Re: Small docstring improvements

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 20 Aug 2016 15:59:54 +0000
> Cc: emacs-devel@gnu.org
> 
>  If we want to give advice like that, IMO we should also give such an
>  advice for when BUFFER is non-nil.
> 
> OK, then I'd append ", and to count the number of characters
> in some other BUFFER, use
> `(with-current-buffer BUFFER (- (point-max) (point-min)))'.". 

Fine with me, thanks.

Re: Small docstring improvements

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 20. Aug. 2016 um 18:11 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sat, 20 Aug 2016 15:59:54 +0000
> > Cc: emacs-devel@gnu.org
> >
> >  If we want to give advice like that, IMO we should also give such an
> >  advice for when BUFFER is non-nil.
> >
> > OK, then I'd append ", and to count the number of characters
> > in some other BUFFER, use
> > `(with-current-buffer BUFFER (- (point-max) (point-min)))'.".
>
> Fine with me, thanks.
>

Thanks, pushed.

[-- Attachment #2: Type: text/html, Size: 995 bytes --]

Re: Small docstring improvements

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> Once again, I don't see why you dislike that.  Lisp is not C.

It's like having a function called "print" that sends e-mail when called on
Tuesday. Yay, polymorphism! But it doesn't mean what it says on the box.

START should mean START, or else the function shouldn't be called
`call-process-region' (since it's not always acting on a region, sometimes
it's a string), and the argument shouldn't be called START, but rather
START-OR-BUFFER-IF-NIL-OR-STRING, since that's what it actually means.

I'm against one thing acting as multiple other things, just because a hack of
that nature was convenient at some time in the past. It's ugly, it's messy,
it's unnecessary. If a function does 3 separate-yet-related things, there
should be 3 separate-yet-related functions instead.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2