Thread: Re: 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 06.09.2024 16:20, Daniel Gustafsson wrote: >> 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 > > >
> 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.
> 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
> 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 On 02.10.2024 15:58, Daniel Gustafsson wrote: >> 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 > > >