Re: Documentation improvement patch - Mailing list pgsql-docs

> On 1 Oct 2024, at 10:04, Oleg Sibiryakov <o.sibiryakov@postgrespro.ru> wrote:

> Here is a kind reminder of a small documentation improvement patch, which we started discussing a month ago.
>
> I removed all the controversial points touched upon in this thread. Please, take a look once again at your
convenience.

In general, when submitting a docs patch it's better to not reflow the
paragraphs when a modified line becomes too long.  Reading a 4 line diff where
only one thing changed in the first becomes harder than reading a single line
diff where the line is long.  The committer can ensure the lines are reflowed
prior to a commit, or it can be left as the final revision of a patch
submission once all changes are discussed-

A few comments on this version of the patch:


-       ICU<indexterm><primary>ICU</primary></indexterm>
+       <indexterm><primary>ICU</primary></indexterm>

I don't think removing the name of the library changing the sentence from "The
icu provider uses the external ICU library" to "The icu provider uses the
external library" is an improvement.


-   by sequences.  (See <xref linkend="sql-createsequence"/>.)  The properties
+   by sequences (see <xref linkend="sql-createsequence"/>).  The properties

This is a common construction in our docs, if it's considered to be a bad
practice the case should be argued (separately) for removing all of them
instead.


-       Comma separated list of publication names for which to subscribe
+       Comma-separated list of publication names for which to subscribe

There are two more cases of "comma separated" (config.sgml and copy.sgml),
should they be changed too?


-          the failover if required, enable the subscription, and refresh the
-          subscription. See
+          the <literal>failover</literal> if required, enable the subscription,
+          and refresh the subscription. See

This refers to the act of failing over, not the property value failover, and
should not be in <literal>.


-    for not-null constraints at all, so they are not
+    for <literal>NOT NULL</literal> constraints at all, so they are not

I'm still not convinced that this change makes the documentation more readable.


-       the <command>MERGE</command> command will perform a <literal>FULL</literal>
-       join between <replaceable class="parameter">data_source</replaceable>
-       and the target table.  For this to work, at least one
+       the <command>MERGE</command> command will perform a
+       <literal>FULL JOIN</literal> between
+       <replaceable class="parameter">data_source</replaceable> and the target
+       table. For this to work, at least one

This paragraph discuss various join types, keeping it lowercase "join" matches
the remainder of the paragraph and makes it more readable IMHO.  It's not
discussing syntax the user is expected to type so need to make it so.

--
Daniel Gustafsson