Thread: plpgsql-trigger.html: Format TG_ variables as table (patch)
Hi, I found the list of TG_ variables on https://www.postgresql.org/docs/current/plpgsql-trigger.html#PLPGSQL-DML-TRIGGER hard to read for several reasons: too much whitespace, all the lines start with "Data type", and even after that, the actual content is hiding behind some extra "variable that..." boilerplate. The attached patch formats the list as a table, and removes some of the clutter from the text. I reused the catalog_table_entry table machinery, that is probably not quite the correct thing, but I didn't find a better variant, and the result looks ok. Thanks to ilmari for the idea and some initial reviews. Christoph
Attachment
On 30.08.22 15:16, Christoph Berg wrote: > I found the list of TG_ variables on > https://www.postgresql.org/docs/current/plpgsql-trigger.html#PLPGSQL-DML-TRIGGER > hard to read for several reasons: too much whitespace, all the lines > start with "Data type", and even after that, the actual content is > hiding behind some extra "variable that..." boilerplate. > > The attached patch formats the list as a table, and removes some of > the clutter from the text. > > I reused the catalog_table_entry table machinery, that is probably not > quite the correct thing, but I didn't find a better variant, and the > result looks ok. I find the new version even harder to read. The catalog_table_entry stuff doesn't really make sense here, since what you have before is already a definition list, and afterwards you have the same, just marked up "incorrectly". We could move the data type in the <term>, similar to how you did it in your patch. I agree the whitespace layout is weird, but that's a problem of the website CSS stylesheet. I think it looks a bit better with the local stylesheet, but that can all be tweaked.
> On 31 Aug 2022, at 11:35, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 30.08.22 15:16, Christoph Berg wrote: >> I found the list of TG_ variables on >> https://www.postgresql.org/docs/current/plpgsql-trigger.html#PLPGSQL-DML-TRIGGER >> hard to read for several reasons: too much whitespace, all the lines >> start with "Data type", and even after that, the actual content is >> hiding behind some extra "variable that..." boilerplate. >> The attached patch formats the list as a table, and removes some of >> the clutter from the text. >> I reused the catalog_table_entry table machinery, that is probably not >> quite the correct thing, but I didn't find a better variant, and the >> result looks ok. > > I find the new version even harder to read. The catalog_table_entry stuff doesn't really make sense here, since what youhave before is already a definition list, and afterwards you have the same, just marked up "incorrectly". If we change variable lists they should get their own formatting in the xsl and css stylesheets. > We could move the data type in the <term>, similar to how you did it in your patch. That will make this look different from the trigger variable lists for other languages (which typically don't list type), but I think it's worth it to avoid the boilerplate which is a bit annoying. Another thing we should change while there (but it's not directly related to this patch) is that we document TG_RELID and $TG_relid as "object ID" but TD["relid"] and $_TD->{relid} as "OID". Punctuation of item descriptions is also not consistent. -- Daniel Gustafsson https://vmware.com/
Re: Peter Eisentraut > I find the new version even harder to read. The catalog_table_entry stuff > doesn't really make sense here, since what you have before is already a > definition list, and afterwards you have the same, just marked up > "incorrectly". Fair enough. For comparison, this is what yesterday's patch looked like: https://www.df7cb.de/s/2022-08-31.115813.w5UvAS.png > We could move the data type in the <term>, similar to how you did it in your > patch. The new version of the patch just moves up the data types, and removes the extra clutter from the beginnings of each description: https://www.df7cb.de/s/2022-08-31.115857.LkkKl8.png Christoph
Attachment
Re: To Peter Eisentraut > The new version of the patch just moves up the data types, and removes > the extra clutter from the beginnings of each description: The last version had the brackets in TG_ARGV[] (text[]) duplicated. Christoph
Attachment
> On 31 Aug 2022, at 13:55, Christoph Berg <myon@debian.org> wrote:
>
> Re: To Peter Eisentraut
>> The new version of the patch just moves up the data types, and removes
>> the extra clutter from the beginnings of each description:
>
> The last version had the brackets in TG_ARGV[] (text[]) duplicated.
This, and the other string variables, now reads a bit strange IMO:
- Data type <type>text</type>; a string of
+ string
<literal>INSERT</literal>, <literal>UPDATE</literal>,
<literal>DELETE</literal>, or <literal>TRUNCATE</literal>
Wouldn't it be better with "string containing <literal>INSERT.." or something
similar?
--
Daniel Gustafsson https://vmware.com/
Re: Daniel Gustafsson > This, and the other string variables, now reads a bit strange IMO: > > - Data type <type>text</type>; a string of > + string > <literal>INSERT</literal>, <literal>UPDATE</literal>, > <literal>DELETE</literal>, or <literal>TRUNCATE</literal> > > Wouldn't it be better with "string containing <literal>INSERT.." or something > similar? Right, that felt strange to me as well, but I couldn't think of something better. "string containing" is again pretty boilerplatish, how about just "contains"? Christoph
Attachment
Re: To Daniel Gustafsson
> "string containing" is again pretty boilerplatish, how about just
> "contains"?
Actually, just omitting the whole prefix works best.
TG_WHEN (text)
BEFORE, AFTER, or INSTEAD OF, depending on the trigger's definition.
I also shortened some "name of table" to just "table". Since the data
type is "name", it's clear what "table" means.
Christoph
Attachment
Christoph Berg <myon@debian.org> writes: > Re: To Daniel Gustafsson >> "string containing" is again pretty boilerplatish, how about just >> "contains"? > > Actually, just omitting the whole prefix works best. > > TG_WHEN (text) > > BEFORE, AFTER, or INSTEAD OF, depending on the trigger's definition. The attached patch does not reflect this, did you attach an old version? > I also shortened some "name of table" to just "table". Since the data > type is "name", it's clear what "table" means. I think it reads better with the definite article and initial capital, e.g. "The table that triggered ….". > <variablelist> > <varlistentry> > - <term><varname>NEW</varname></term> > + <term><varname>NEW</varname> (record)</term> The type names should still be wrapped in <type>, like they were before. - ilmari
Re: Dagfinn Ilmari Mannsåker > > Actually, just omitting the whole prefix works best. > > > > TG_WHEN (text) > > > > BEFORE, AFTER, or INSTEAD OF, depending on the trigger's definition. > > The attached patch does not reflect this, did you attach an old version? Forgot to git commit before exporting the patch, thanks for catching! > > I also shortened some "name of table" to just "table". Since the data > > type is "name", it's clear what "table" means. > > I think it reads better with the definite article and initial capital, > e.g. "The table that triggered ….". Since that's not a complete sentence anyway, I think "The" isn't necessary. > > - <term><varname>NEW</varname></term> > > + <term><varname>NEW</varname> (record)</term> > > The type names should still be wrapped in <type>, like they were before. Updated. Thanks, Christoph
Attachment
> On 1 Sep 2022, at 15:07, Christoph Berg <myon@debian.org> wrote: > Re: Dagfinn Ilmari Mannsåker >>> I also shortened some "name of table" to just "table". Since the data >>> type is "name", it's clear what "table" means. >> >> I think it reads better with the definite article and initial capital, >> e.g. "The table that triggered ….". > > Since that's not a complete sentence anyway, I think "The" isn't > necessary. Looking at the docs for the other PLs there is quite a lot of variation on how we spell this, fixing that inconsistency is for another patch though. The patch missed to update the corresponding list for TG_ event trigger vars, fixed in the attached. -- Daniel Gustafsson https://vmware.com/
Attachment
> On 2 Sep 2022, at 11:19, Daniel Gustafsson <daniel@yesql.se> wrote: > The patch missed to update the corresponding list for TG_ event trigger vars, > fixed in the attached. I took another look at this, and pushed it with a few small tweaks. Thanks! -- Daniel Gustafsson https://vmware.com/