On Sat, Jan 10, 2026 at 4:46 PM John Naylor <[email protected]> wrote:
> On Wed, Jan 7, 2026 at 3:33 PM albert tan <[email protected]> wrote: > > Hi Join, > > Thanks for pointing out extra issues. I have addressed them in v2. > > Thanks, but I can't use this -- it looks like you just picked the > first difference you saw in my proposed edits and ignored the rest, > leaving the result just as awkward or ungrammatical as before (see > below). That doesn't save me any time. > > > >/* > > > * The directory parameter can be omitted, absolute, or relative to the > > > * installation's base directory, which can be the sharedir or a custom > > > * path that it was set extension_control_path. It depends where the > > > * .control file was found. > > > */ > > > > > >I imagine this means "or a custom path that was set via > extension_control_path". > > - * path that it was set extension_control_path. It depends where the > + * path that was set extension_control_path. It depends where the > > And, while looking again, there's another error here: "depends on" > > > >/* > > > * Return a list of directories declared on extension_control_path GUC. > > > */ > > > > >I would guess this normally phrased something like "Return the list of > > >directories in extension_control_path". > > - * Return a list of directories declared on extension_control_path GUC. > + * Return the list of directories declared on extension_control_path GUC. > > My main gripe here was "declared on <foo> GUC", not the article, which > on second thought may as well remain "a list" since we do use that > elsewhere to mean "an instance of the List type". I went with "Return > a list of directories declared in the extension_control_path GUC.". > > Also, I decided that the paragraph that started this report needs to > be rewritten: > > > ' The control file will be search on Extension_control_path paths if > > control->control_dir is NULL, otherwise it will use the value of > control_dir > > to read and parse the .control file, so it assume that the control_dir > is a > > valid path for the control file being parsed.' > > - I'm not sure what the "assume" phrase is explaining, since we always > check for errors when opening a file. The contract elsewhere for > filling in control_dir doesn't seem as relevant as the NULL behavior. > - Passive voice is often used to maintain discourse cohesion, but the > following "it" doesn't refer to anything in particular. > > On Fri, Jan 9, 2026 at 1:17 PM li carol <[email protected]> wrote: > > This should be corrected to: > > > > "Works in a very similar way to find_in_path, but it receives an already > parsed List of paths to search the basename, and it does not support macro > replacement or custom error messages (for simplicity). By 'already parsed > List of paths' this function expects that the paths already have all macros > replaced." > > Thanks, but I see more that should be done here (also, for something > this long it's better to just mention what the corrections are, or > write a patch, so I don't have to figure it out). The original comment > is awkwardly written in that it has a separate paragraph to explain > what a term in the first paragraph means, so I simplified it to say > what it means. > > This has now gone beyond a trivial fix, so I've attached what I > propose to commit barring other opinions. > > -- > John Naylor > Amazon Web Services > Hi John, v4-0001 looks good to me. I just went through extension.c and got a few more comment improvements. I put them in v5-0002, please take a look. Feel free to squash. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v5-0001-Improve-some-comment-wording-and-grammar-in-exten.patch
Description: Binary data
v5-0002-A-few-more-comment-improves-in-extension.c.patch
Description: Binary data
