public inbox for [email protected]  

help / color / mirror / Atom feed

From: Fabien COELHO <[email protected]>
To: Justin Pryzby <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: David Steele <[email protected]>
Cc: [email protected]
Cc: Bossart, Nathan <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Date: Sun, 12 Apr 2020 13:53:40 +0200 (CEST)
Message-ID: <alpine.DEB.2.21.2004121145190.5257@pseudo> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<alpine.DEB.2.21.2003170828250.17584@pseudo>
	<[email protected]>
	<[email protected]>


Hello Justin,

About v15, seen as one patch.

Patches serie applies cleanly, compiles, "make check" ok.

Documentation:
  - indent documentation text around 80 cols, as done around?
  - indent SQL example for readability and capitalize keywords
    (pg_ls_dir_metadata)
  - "For each file in a directory, list the file and its metadata."
    maybe: "List files and their metadata in a directory"?

Code:
  - Most pg_ls_*dir* functions call pg_ls_dir_files(), which looks like
    reasonable refactoring, ISTM that the code is actually smaller.
  - please follow pg style, eg not "} else {"
  - there is a "XXX" (meaning fixme?) tag remaining in a comment.
  - file types: why not do block & character devices, fifo and socket
    as well, before the unkown case?
  - I'm wondering whether could pg_stat_file call pg_ls_dir_files without
    too much effort? ISTM that the output structure nearly the same. I do
    not like much having one function specialized for files and one for
    directories.

Tests:
  - good, there are some!
  - indent SQL code, eg by starting a new line on new clauses?
  - put comments on separate lines (I'm not against it on principle, I do
    that, but I do not think that it is done much in test files).

-- 
Fabien.

view thread (57+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
  In-Reply-To: <alpine.DEB.2.21.2004121145190.5257@pseudo>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox