Thread: [htmltmpl] PATCH: adding auto-detection for file types
Brought to you by:
samtregar
From: Mark S. <ma...@su...> - 2008-12-01 00:02:26
Attachments:
auto-type.patch
|
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. 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. Mark -- http://mark.stosberg.com/blog/ |
From: Brad B. <bm...@ma...> - 2008-12-03 18:44:53
|
I have a couple of comments below... On Sun, Nov 30, 2008 at 7:02 PM, Mark Stosberg <ma...@su...> 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<http://search.cpan.org/%7Etjmather/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<http://search.cpan.org/%7Etjmather/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 |
From: David K. <da...@gi...> - 2008-12-04 23:42:53
|
Hi Mark, "Mark Stosberg" <ma...@su...> wrote... > > HTML::Template currently supports 4 formats > for the input file, with three different APIs > to specify it [...] I find these more verbose > and confusing than it needs to be. [...] > Before proceeding further, I wanted to get > some feedback about heading in this direction. +1 to this idea! I think this would be a very nice improvement. I always have to consult the docs to use anything but a filename, too. Please finish the patch! As long as it is maintains compatibility with existing code, includes docs and tests, I bet Sam would include it in the next release. -dave |