Thread: small patch

small patch

From
rir
Date:
Minor changes to move.sgml and fetch.sgml.

The text 'or empty' is inconsistent by restating what the
synopsis notation has expressed.

The comments on sharing a language feature, while
likely helpful during review, seem verbose compared to
the non-commenting in other similar files.

Thanks again,
Rob



Attachment

Re: small patch

From
Laurenz Albe
Date:
On Fri, 2021-10-01 at 21:06 -0400, rir wrote:
> Minor changes to move.sgml and fetch.sgml.
> 
> The text 'or empty' is inconsistent by restating what the
> synopsis notation has expressed.
> 
> The comments on sharing a language feature, while
> likely helpful during review, seem verbose compared to
> the non-commenting in other similar files.

Thanks for the effort of preparing a patch.

However, I don't think that is an improvement:

- the comments pointing from MOVE to FETCH and vice versa are
  helpful for people who edit the documentation like you did

- we should retain "empty or one of", otherwise the following syntax
  would be undocumented:

      FETCH FROM c;

Yours,
Laurenz Albe




Re: small patch

From
rir
Date:
On Mon, Oct 04, 2021 at 08:18:22AM +0200, Laurenz Albe wrote:
> On Fri, 2021-10-01 at 21:06 -0400, rir wrote:
> > Minor changes to move.sgml and fetch.sgml.
> > 
> > The text 'or empty' is inconsistent by restating what the
> > synopsis notation has expressed.
> > 
> > The comments on sharing a language feature, while
> > likely helpful during review, seem verbose compared to
> > the non-commenting in other similar files.
> 
> Thanks for the effort of preparing a patch.
> 
> However, I don't think that is an improvement:
>
> - the comments pointing from MOVE to FETCH and vice versa are
>   helpful for people who edit the documentation like you did

> - we should retain "empty or one of", otherwise the following syntax
>   would be undocumented:
>
>       FETCH FROM c;

> Yours,
> Laurenz Albe

Laurenz,

Your view is completely reasonable, but it suggests that
many of the synopses are leaving syntax undocumented.
The 'empty or one of:' is only used in these two synopses.

If I had found the comments helpful, I would have spared them.
My sense was that the comments were unusual by their existence
for their purpose.  That was not rigorous:  so okay.

Since I'm only making the same points again, I'll stop.

Thanks again,
Rob



Re: small patch

From
Laurenz Albe
Date:
On Wed, 2021-10-06 at 23:39 -0400, rir wrote:
> On Mon, Oct 04, 2021 at 08:18:22AM +0200, Laurenz Albe wrote:
> > On Fri, 2021-10-01 at 21:06 -0400, rir wrote:
> > > Minor changes to move.sgml and fetch.sgml.
> > > 
> > > The text 'or empty' is inconsistent by restating what the
> > > synopsis notation has expressed.
> > > 
> > > The comments on sharing a language feature, while
> > > likely helpful during review, seem verbose compared to
> > > the non-commenting in other similar files.
> > 
> > Thanks for the effort of preparing a patch.
> > 
> > However, I don't think that is an improvement:
> > 
> > - the comments pointing from MOVE to FETCH and vice versa are
> >   helpful for people who edit the documentation like you did
> > - we should retain "empty or one of", otherwise the following syntax
> >   would be undocumented:
> > 
> >       FETCH FROM c;
> 
> Your view is completely reasonable, but it suggests that
> many of the synopses are leaving syntax undocumented.
> The 'empty or one of:' is only used in these two synopses.

You have a point there.

Can you think of a way to modify the syntax diagram so that it
expresses that and still remains comprehensible?

Yours,
Laurenz Albe




Re: small patch

From
rir
Date:
On Thu, Oct 07, 2021 at 07:58:47AM +0200, Laurenz Albe wrote:
> On Wed, 2021-10-06 at 23:39 -0400, rir wrote:
> > On Mon, Oct 04, 2021 at 08:18:22AM +0200, Laurenz Albe wrote:
> > > On Fri, 2021-10-01 at 21:06 -0400, rir wrote:

> > > > Minor changes to move.sgml and fetch.sgml.

> > > However, I don't think that is an improvement:
> > > 
> > > - the comments pointing from MOVE to FETCH and vice versa are
> > >   helpful for people who edit the documentation like you did
> > > - we should retain "empty or one of", otherwise the following syntax
> > >   would be undocumented:
> > > 
> > >       FETCH FROM c;
> > 
> > Your view is completely reasonable, but it suggests that
> > many of the synopses are leaving syntax undocumented.
> > The 'empty or one of:' is only used in these two synopses.

> Can you think of a way to modify the syntax diagram so that it
> expresses that and still remains comprehensible?


For myself,
   'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
clearly indicates that 'direction' is optional.

Rob



Re: small patch

From
Tom Lane
Date:
rir <rirans@comcast.net> writes:
> On Thu, Oct 07, 2021 at 07:58:47AM +0200, Laurenz Albe wrote:
>> Can you think of a way to modify the syntax diagram so that it
>> expresses that and still remains comprehensible?

> For myself,
>    'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
> clearly indicates that 'direction' is optional.

Right, but if we don't say that <direction> can be
empty, then this diagram disallows

    FETCH FROM cursor_name

which in fact is legal.  I think however that we could make it read

    FETCH [ <direction> ] [ FROM | IN ] <cursor_name>'

and have a correct description without requiring <direction>
to be allowed to be empty.

BTW, as it stands, the diagram is ambiguous
because there are two ways to parse

    FETCH cursor_name

... is <direction> present but empty, or omitted altogether?

            regards, tom lane



Re: small patch

From
Laurenz Albe
Date:
On Thu, 2021-10-07 at 16:06 -0400, rir wrote:
> > > > - we should retain "empty or one of", otherwise the following syntax
> > > >   would be undocumented:
> 
> For myself,
>    'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
> clearly indicates that 'direction' is optional.

Yes, but since [ FROM | IN ] is inside the bracket, that
diagram would not account for "direction" being omitted
while retaining FROM.

So I suggest that you change the syntax diagram to

FETCH [ direction ] [ FROM | IN ] cursor_name

Then I agree that the "empty or" can be removed.
I think that would be a good idea, and it would add
clarity.

I remain of the opinion that the comments should be
retained, but we can leave that for somebody else to
decide.

Do you want to prepare a new patch and register it in
the commitfest?

Yours,
Laurenz Albe




Re: small patch

From
Tom Lane
Date:
rir <rirans@comcast.net> writes:
> On Thu, Oct 07, 2021 at 04:24:16PM -0400, Tom Lane wrote:
>> BTW, as it stands, the diagram is ambiguous
>> because there are two ways to parse
>> FETCH cursor_name
>> ... is <direction> present but empty, or omitted altogether?

> I am confident you know what you mean, but I don't.  At the _parsing_
> stage how is any distinction between emptiness and omission?

Bison certainly makes a distinction, because it would have two different
production paths it could follow if the actual grammar looked like this
(which it doesn't).  It doesn't care whether the emitted parse tree
would be the same.  Indeed, it would be possible for the two cases to
create different parse trees.

            regards, tom lane



Re: small patch

From
rir
Date:
On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:
> On Thu, 2021-10-07 at 16:06 -0400, rir wrote:


> So I suggest that you change the syntax diagram to
>
> FETCH [ direction ] [ FROM | IN ] cursor_name

> Then I agree that the "empty or" can be removed.

> I remain of the opinion that the comments should be
> retained, but we can leave that for somebody else to
> decide.

I accept your three points above.

The MOVE synopsis shows the same parsing as I presented,
should it be changed in the same way (move a square bracket left to
be after <direction>)?  My guess is yes, but I've never used an
SQL cursor.

When this convo settles, I send a new patch.  Probably
here in the group.  If I have a few more, or a complex one,
I'll check out the other submission method.

Rob




Re: small patch

From
Laurenz Albe
Date:
On Sun, 2021-10-10 at 16:15 -0400, rir wrote:
> On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:
> > On Thu, 2021-10-07 at 16:06 -0400, rir wrote:
> 
> 
> > So I suggest that you change the syntax diagram to
> > 
> > FETCH [ direction ] [ FROM | IN ] cursor_name
> 
> > Then I agree that the "empty or" can be removed.
> 
> > I remain of the opinion that the comments should be
> > retained, but we can leave that for somebody else to
> > decide.
> 
> I accept your three points above.
> 
> The MOVE synopsis shows the same parsing as I presented,
> should it be changed in the same way (move a square bracket left to
> be after <direction>)?  My guess is yes, but I've never used an
> SQL cursor.
> 
> When this convo settles, I send a new patch.  Probably
> here in the group.  If I have a few more, or a complex one,
> I'll check out the other submission method.

Yes, I think that such a patch would meet with favor.

Make sure to register it on the commitfest.  To do that, it is better
to send the patch to the -hackers list.  The commitfest application
won't find conversations on the -docs list.

Yours,
Laurenz Albe




Re: small patch

From
Laurenz Albe
Date:
On Mon, 2021-10-11 at 23:14 -0400, rir wrote:
> On Sun, Oct 10, 2021 at 04:15:01PM -0400, rir wrote:
> > On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:
> > > On Thu, 2021-10-07 at 16:06 -0400, rir wrote:
> 
> > > FETCH [ direction ] [ FROM | IN ] cursor_name
> 
> Should the pgplsql docs be similiarly changed per the above
> in 43.7.3.1 and 2?

Absolutely!

It may even make sense to extend the existing comments to
alert people who modify the syntax diagram in the future
that they should also consider reviewing the PL/pgSQL syntax.

Yours,
Laurenz Albe




Re: small patch

From
rir
Date:
On Tue, Oct 12, 2021 at 08:43:34AM +0200, Laurenz Albe wrote:
> On Mon, 2021-10-11 at 23:14 -0400, rir wrote:
> > On Sun, Oct 10, 2021 at 04:15:01PM -0400, rir wrote:
> > > On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:
> > > > On Thu, 2021-10-07 at 16:06 -0400, rir wrote:
> > 
> > > > FETCH [ direction ] [ FROM | IN ] cursor_name
> > 
> > Should the pgplsql docs be similiarly changed per the above
> > in 43.7.3.1 and 2?
> 
> Absolutely!
> 
> It may even make sense to extend the existing comments to
> alert people who modify the syntax diagram in the future
> that they should also consider reviewing the PL/pgSQL syntax.

I will make the syntax change.

Since my understanding is that, absent contrary documentation,
plpgSQL follows SQL, I resist the idea of yet another comment.
Better would be establishing a SPoT, tagging it, and tagging
various forms of derived info as quotes, code samples, etc. or,
at worst, logically dependent.  This more form of commenting
could allow linting processes to support its use and check
its accuracy.

Absent that ...

Rob





Re: small patch

From
Alvaro Herrera
Date:
On 2021-Oct-08, Laurenz Albe wrote:

> I remain of the opinion that the comments should be
> retained, but we can leave that for somebody else to
> decide.

So I just realized that I added this comment in 8c250f3741f.

The point of this comment is that the list of options to which
"direction" expands is duplicate and needs to be kept in sync.  However,
clearly we have other places in the docs where similar lists are
duplicated and no such comments are kept; see "column_constraint" in
alter_table.sgml and create_table.sgml for an obvious one.  I can't
quite make up my mind about the comment being actually helpful if
somebody adds a new type of contraints to remind them that they also
need to add it to the other place.  What do you think?

I think I side with Laurenz that the comment should be kept, even if
it's just out of inertia.

Maybe a better solution (more convoluted? overengineered?) would be to
define an SGML entity in the first of those pages that uses the list in
such a way so that it expands to that list, and then use that entity in
the other page.  I don't know how that is done, but surely it must be
possible.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)