[Petal] [PATCH] Options via constructor rather than package globals

[Grant McLean]

Jean-Michel Hiver wrote:
>>1. a base class for all Petal objects, ie: Petal::Object, or
>>   Petal::Base.  To begin with, this would contain new() and
>>   _initialize(), which are easily overrided.
> 
> 
> I'm not sure if I agree with the concept of 'base classes'.

I don't really object to this one, but I don't quite get the
point either.

>>2. cascade-able accessor methods which let you do:
>>
>>	$template
>>	  ->language('fr')
>>	  ->input('HTML')
>>	  ->output('XHTML');
> 
> Now I find this really, really useless :)
> 
> I disagree with the idea of constructing an object that is in
 > an inconsistent state and setting the state afterwards through
 > some strange syntaxic sugar.

I tend to agree with Jean Michel on this one - for exactly the same
reasons.  I have seen it before but don't find it at all intuitive.

>>3. use accessors within _initialize().  In the off-chance that
>>   someone comes along and sub-classes Petal :)
>>
>>Additionally, you might get rid of a lot of cut-n-paste work in 
>>_initialize() by doing something like:
>>
>>	...
>>	foreach my $param (qw( taint disk_cache ... )) {
>>        no strict 'refs';
>>		my $val = exists $o{$param}
>>		  ? $o{param}
>>		  : ${'Petal::' . uc(param)
>>		$self->$param( $val );
>>	}

I actually started out with something a bit like that but the
exceptions cases started to mount up and I decided the cut-n-
paste version was clearer (although some of the lines ended up
uncomfortably long).

>>Again, good work.  Lets hope Jean-Michel agrees ;-)
> 
> I applied the patch, looked at it, and then tweaked a little bit things
> the way I wanted...
> 
> Having the options at an object level did really involve quite a lot of
> changes. 

That's why it took me so long :-)

Apart from all the added initialisation code, I felt it actually
simplified things in most places changes were required.

> So instead of making the rest of Petal code aware of per-object
> options, I changed the process() method to declare the global options
> variables as local and set them to the per-objects options.

I was aiming to have none of the other modules directly access any
of the configuration globals in the Petal:: namespace.  But I can
understand that the scale of the changes could make you nervous.

> It might seem like a bit of a hack but I think it's acceptable and it
> fits in better with the way Petal was designed to start with. Also, the
> patch seemed to fail one test...

It was passing all the tests when it left here :-)  Did the patch
apply cleanly?  I haven't generated patches much before so I may have
screwed that up.

> Anyway since I haven't released Petal in a while I'll to a .93 release
> that should sort out the most urgent stuff (including the
> Petal::Hash::VAR bug you pointed out). Meanwhile more deep changes to
> Petal will have to wait...
> 
> Cheers,

Thanks
Grant