Re: cleaning up PostgresNode.pm

On 4/24/21 3:14 PM, Alvaro Herrera wrote:
> 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().
>

Here is a set of small(ish) patches that does most of the above and then
some.

Patch 1 adds back the '-w' flag to pg_ctl in the start() method. It's
redundant on modern versions of Postgres but it's harmless, and helps
with subclassing for older versions where it wasn't the default.

Patch 2 adds a method for altering config files as opposed to just
appending to them. Again, this helps a lot in subclassing for older
versions, which can call the parent's init() and then adjust whatever
doesn't work.

Patch 3 unifies the constructor methods and stops exporting a
constructor. There is one constructor: PostgresNode::new()

Patch 4 removes what's left of Exporter in PostgresNode, so it becomes a
pure OO style module.

Patch 5 adds a method for getting the major version string from a
PostgresVersion object, again useful in subclassing.

Patch 6 adds a method for getting the install_path of a PostgresNode
object. While not strictly necessary it's consistent with other fields
that have getter methods. Clients should not pry into the internals of
objects. Experience has shown this method to be useful.

Patches 7 8 and 9 contain additions to Patch 3 for things that I
overlooked or that were not present when I originally prepared the
patches. They would be applied alongside Patch 3, not separately.

These patches are easily broken by e.g. the addition of a new TAP test
or the modification of an existing test. So I'm hoping to get these
added soon. I will add this email to the CF.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com