Date: Sat, 24 Apr 2021 15:14:44 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: cleaning up PostgresNode.pm
Message-ID: <20210424191444.GA25008@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5fedb05a-0d0d-d721-2e7c-1ba99919f87d@dunslane.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210424191444.GA25008%40alvherre.pgsql>
Precedence: bulk

On 2021-Apr-24, Andrew Dunstan wrote:

> 
> I would like to undertake some housekeeping on PostgresNode.pm.
> 
> 1. OO modules in perl typically don't export anything. We should remove
> the export settings. That would mean that clients would have to call
> "PostgresNode->get_new_node()" (but see item 2) and
> "PostgresNode::get_free_port()" instead of the unadorned calls they use now.

+1

> 2. There are two constructors, new() and get_new_node(). AFAICT nothing
> in our tests uses new(), and they almost certainly shouldn't anyway.
> get_new_node() calls new() to do some work, and I'd like to merge these
> two. The name of a constructor in perl is conventionally "new" as it is
> in many other OO languages, although in perl this can't apply where a
> class provides more than one constructor. Still, if we're merging them
> then the preference would be to call the merged function "new". Since
> we'd proposing to modify the calls anyway (see item 1) this shouldn't
> impose a huge extra workload.

+1 on "new".  I think we weren't 100% clear on where we wanted it to go
initially, but it's now clear that get_new_node() is the constructor,
and that new() is merely a helper.  So let's rename them in a sane way.

> Another item that needs looking at is the consistent use of Carp.
> PostgresNode, TestLib and RecursiveCopy all use the Carp module, but
> contain numerous calls to "die" where they should probably have calls to
> "croak" or "confess".

I wonder if it would make sense to think of PostgresNode as a feeder of
sorts to Test::More and the like, so make it use diag(), note(),
explain().

-- 
Álvaro Herrera       Valdivia, Chile
"If you have nothing to say, maybe you need just the right tool to help you
not say it."                   (New York Times, about Microsoft PowerPoint)