I have a couple of comments below...

On Sun, Nov 30, 2008 at 7:02 PM, Mark Stosberg <mark@summersault.com> wrote:

HTML::Template currently supports 4 formats for the input file, with three different APIs
to specify it:

 First:
  new( filename   => $f, ... );
  new( scalarref  => $f, ... );
  new( arrayref   => $f, ... );
  new( filehandle => $f, ... );

 Second:
  new_file($f, ... );
  new_scalar_ref($f, ... );
  new_array_ref($f, ... );
  new_filehandle($f, ... );

 Third:
  new(type => 'filename'  , source => $f, ...);
  new(type => 'scalarref' , source => $f, ...);
  new(type => 'arrayref'  , source => $f, ...);
  new(type => 'filehandle', source => $f, ...);

###

I find these more verbose and confusing than it needs to be. Beecause the
second option provides alternate spellings for 3 out of 4 options, it's hard to
ever get the spelling right without consulting the docs.

First, I'd like to see alternatives for the unfortunately misspelled :-) methods:

new_filename()
new_scalarref()
new_arrayref()

Since those are practically one-liners anyway, a few additions ought not be
a big deal.

 

I would like to contribute a patch that cleans all this up to this:

 new(source => $filehandle);
 new(source => $filename);
 new(source => $scalarref);
 new(source => $arrayref);

Since we can detect the difference between a string, a scalarref, an arrayref
and a filehandle, we don't need to user to repeat this information for us. It
can "just work". The auto-detection logic would come into play if "source" is
specified, but 'type' is not.

HTML::FillInForm had a similar interface and is a precendent for this kind of API overhaul.
You can see their old API:

http://search.cpan.org/~tjmather/HTML-FillInForm-2.00/lib/HTML/FillInForm.pm#Old_syntax

And the new API:

http://search.cpan.org/~tjmather/HTML-FillInForm-2.00/lib/HTML/FillInForm.pm#SYNOPSIS

The attach patch includes working code and tests which head in this direction,
but it is not complete. (The current patch auto-detects the first arg, instead of
the value for 'source').

The final patch would update the docs to highlight the simpler new syntax and
de-emphasize the other interfaces (The same approach used by HTML::FillInForm).
This would simplify things for new users, while users migrating from older
versions would still find the reference docs for the older APIs.

Before proceeding further, I wanted to get some feedback about heading in this
direction.

I think this is an okay idea.  But I'm leery of the technique of shoehorning
alternative syntax over existing syntax the way your patch seems to imply.

Instead, I would look at this spot in the code:
  # handle the "type", "source" parameter format (does anyone use it?)
if (exists($options->{type})) {
exists($options->{source}) or croak("HTML::Template->new() called with 'type' parameter set, but no 'source'!");
($options->{type} eq 'filename' or $options->{type} eq 'scalarref' or
$options->{type} eq 'arrayref' or $options->{type} eq 'filehandle') or
croak("HTML::Template->new() : type parameter must be set to 'filename', 'arrayref', 'scalarref' or 'filehandle'!");

$options->{$options->{type}} = $options->{source};
delete $options->{type};
delete $options->{source};
}
It looks like this is the only place that needs to change.

Finally, at least one other package, HTML::Template::Compiled, may
also benefit from these API changes.

Regards,

Brad