Re: Streamlined application/script development [Re: coding an Application in perl]

On 09/12/2017 07:22 AM, Jean-Louis Martineau wrote:
> Can we use a different hash to store the values? Which means pass a new
> hash ref between all declare_*options, *_property_setter and new?
> The advantage is that it remove the opt_ hack.

Hmm. Making every caller tote another argument around seems like a
steep price for improving aesthetics of opt_. But there could be
another way.

I guess what qualifies it as a 'hack' is that it has consequences
the caller needs to understand (can't invent an option named opt_foo,
and have to store values in opt_foo sometimes instead of foo). If the
details were encapsulated and did not restrict the caller's behavior,
it would be an 'internal implementation detail' rather than a 'hack'.

I should have thought of this right at first, but if I had written
the exact same code but, instead of 'opt_', used a prefix that could
never be part of a Getopt::Long option name, that would eliminate any
arbitrary limitation on what option names the caller can invent.

Then I would just need a set_option_value method that knows where
to store the value, so the caller isn't bothered. Already, new()
takes care of how to retrieve the value, so the caller isn't bothered.

That would completely encapsulate the implementation, and be a
very lightweight change, without more arguments to pass around.

> Can we add a default value to boolean_property_setter?
> That way, caller only need to test for 0 or 1, they do not need to test
> for undef.

I don't think a _setter would be an effective place to set a default,
as it never gets called if the option isn't present.

In amzfs-holdsend, you can see how I made a default for the
UNCOMPRESSED property, which should default true. I didn't bother
setting false defaults for the other boolean properties, because
undef tests false anyway.

Here again, if I make the change above, setting the default would
look more like set_option_value($refopthash, 'uncompressed', 1)
instead of having the 'opt_' prefix exposed.

> In declare_restore_options, I think we can use boolean_property_setter
> for the DAR option.

Sure could. In actual historical sequence, I added the DAR option
before I added boolean_property_setter, but of course it would work.

On the other hand, would it be an improvement? I see the value of
boolean_property_setter chiefly for application writers adding
custom properties. Admins will end up writing those properties in
amanda.conf or disklist, so it's nice to let them spell it various
ways, and nice to have a common setter so six different applications
don't have six slightly different rules.

But if --dar is something only passed from internal Amanda components
and the code in Extract.pm and extract_list.c is known to only use YES,
then boolean_property_setter could be overkill. One could even make the
argument that this is part of a defined protocol between internal
Amanda components, and so should be parsed strictly. (Looking most
strictly, it appears even NO is not a possible value; the option
is either YES or it is not present.)

-Chap