Date: Wed, 23 Jan 2019 13:58:07 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Amit Khandekar <amitdkhan.pg@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: ArchiveEntry optional arguments refactoring
Message-ID: <201901231658.fls2whf2xypv@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190123164734.cnt7ho34ygwsav7z@alap3.anarazel.de>
User-Agent: NeoMutt/20180716-1013-2c1e88
Precedence: bulk

Hello

On 2019-Jan-23, Andres Freund wrote:

> > All the arguments, except Archive, CatalogId and DumpId I've moved
> > into the ArchiveOpts structure. Not all of them could be empty before, but
> > anyway it seems better for consistency and readability. Some of the arguments
> > had empty string as a default value, I haven't changed anything here yet
> > (although this mixture of NULL and "" in ArchiveEntry looks a bit confusing).
> 
> Probably worth changing at the same time, if we decide to go for it.
> 
> To me this does look like it'd be more maintainable going forward.

It does.  How does pgindent behave with it?

I'd use ArchiveEntryOpts as struct name; ArchiveOpts sounds wrong. Also,
the struct members could use better names -- "defn" for example could
perhaps be "createStmt" (to match dropStmt/copyStmt), and expand "desc"
to "description".

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services