Thread: small patch
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
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
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
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
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
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
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
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
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
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
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
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
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)