Documentation improvement patch

On 5 Sep 2024, at 11:33, Oleg Sibiryakov <o.sibiryakov@postgrespro.ru> wrote:

Dear all,
I have prepared a patch containing some minor inconsistencies in the documentation. Please, take a look.
The inconsistencies were noticed by: Ekaterina Kiryanova, Elena Indrupskaya, Maxim Yablokov, Anna Uraskova, Elena Karavaeva, and me.
We will be looking forward to your feedback.
The patch shall be applied to the REL_17_STABLE branch.

Most of these seem fine, but I need another read-through to digest them fully.
Just a few small comments:

-        Specifies the builtin provider locale for the database default
-        collation order and character classification, overriding the setting
-        <xref linkend="create-database-locale"/>.  The <link
+        Specifies the <literal>builtin</literal> provider locale for the database
+        default collation order and character classification, overriding the
+        setting <xref linkend="create-database-locale"/>.  The <link
and
-        Specifies the locale name when the builtin provider is used. Locale support
-        is described in <xref linkend="locale"/>.
+        Specifies the locale name when the <literal>builtin</literal> provider
+        is used. Locale support is described in <xref linkend="locale"/>.

I don't think this use of "builtin" refers to the config value but rather the
type of locale, so I think it's correct to not use <literal> here.

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

This seems mostly to be a question of taste, I don't think not-null is
incorrect here.

--
Daniel Gustafsson

Thank you for your feedback.

1. Since we do not want to use <literal> here, I suggest we hyphenate it
as "built-in". What's your take on it?
2. Leaving not-null is fine.

--
Oleg Sibiryakov

On 10 Sep 2024, at 13:46, Oleg Sibiryakov <o.sibiryakov@postgrespro.ru> wrote:

1. Since we do not want to use <literal> here, I suggest we hyphenate it as "built-in". What's your take on it?

I think that's the right choice given the hyphenation used in the rest of the
docs. There are a few more places on that same page which should be built-in
rather than builtin to separate the concept from the parameter value.

--
Daniel Gustafsson

On 10.09.24 15:02, Daniel Gustafsson wrote:

On 10 Sep 2024, at 13:46, Oleg Sibiryakov <o.sibiryakov@postgrespro.ru> wrote:

1. Since we do not want to use <literal> here, I suggest we hyphenate it as "built-in". What's your take on it?

I think that's the right choice given the hyphenation used in the rest of the
docs. There are a few more places on that same page which should be built-in
rather than builtin to separate the concept from the parameter value.

I suspect that this would lead to the opposite confusion, people
complaining that the provider is called "builtin" not "built-in".

Arguably, the other providers are also "built in". There are no
user-pluggable providers at this time.

Here is a patch without the builtin/built-in corrections (find attached).

But I still believe the issue should be discussed further.
We actually have two options: it is either a spelling mistake (since
built-in should written with a hyphen), or we miss the <literal> tag
(since it is actually also a value).

So I do think we cannot really leave it as is.

--
Oleg Sibiryakov

Dear all,

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.

The patch shall be applied to the master branch this time.

--
Regards,
Oleg Sibiryakov
Technical Writer
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

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

Thank you for your kind feedback! I will take due note of the comments
in the next documentation patches as well.

I have made all the changes as per your feedback and also corrected
paragraph reflow.

The third version of the patch is attached for your consideration.

--
Oleg Sibiryakov

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

Thank you for your kind feedback! I will take due note of the comments in the next documentation patches as well.

I have made all the changes as per your feedback and also corrected paragraph reflow.

The third version of the patch is attached for your consideration.

Thanks, I have gone over and applied most of these changes. I did leave out a
few (like the libc one) where the current page had multiple different versions.

--
Daniel Gustafsson

Thank you, Daniel.

--
Oleg Sibiryakov