On Tue, May 29, 2012 at 4:42 PM, Shlomi Fish <shlo...@shlomifish.org> wrote:
> 1. Regarding the choice of licence, see:
>
I have changed the license to MIT, which was my original idea but then I
saw some other
modules having the combo of Artistic+GPL.
I agree with your arguments, better to stick to MIT.
> 2. %WriteMakefileArgs in "Makefile.PL" is indented by 12 spaces. Why so
> many?
> You also have some three-line spaces in that file, which is also uncommon.
>
Good point, thanks! Fixed.
>
> 3. bin/example.pl should really be under eg/ or examples/ or whatever.
>
Good point, thanks! Fixed.
>
> 4. In the same file, "use strict;" should be separated from the shabang
> with an
> empty line.
Good point, thanks! Fixed.
>
5. my ($name) = $AUTOLOAD; ==> no need for the parentheses here (I don't
> think
> they hurt in this case, but they are confusing.)
>
Good point, thanks! Fixed.
>
> 6. my $validate_name_regex = qr/^[a-zA-Z_]+[a-zA-Z0-9_]*$/; ==> you can
> omit the first "+" in this case.
>
Good point, thanks! Fixed.
>
> 7. «$_ =~ $validate_name_regex || croak "invalid format of argument name:
> $_" for @arg_names;»
>
Good point, thanks! Fixed.
>
> I was wondering where "$_" comes to play until I saw the trailing "for"
> (past the 80 columns). I suggest converting it into a foreach loop with a
> lexical variable:
>
> foreach my $name (@arg_names)
> {
> if ($name !~ $validate_name_regex)
> {
> croak...
> }
> }
>
Good point, thanks! Fixed.
>
> 8. You may wish to cache the prepared SQL statements in "call()" based on
> the $name somewhere for making the execution faster (instead of preparing a
> statement each time the function is called. (Though DBI may already do this
> for you).
>
I'm planning on adding support for that later on.
Prepared statements can be a problem if you would happen to change the
function arguments, your prepared statement would then be invalid and the
database would throw an error.
Not a problem, just to replan and try again. But I will fix this later on,
together with some other nice to haves, such as automatically reconnecting
to the database upon a disconnect or communication error and retrying the
query. The retrying should be made an option.
Not entirely sure I want this in the module though, you could argue its
something people want to handle differently and implement outside of the
module.
Will think about it a bit more.
>
> 9. "$proretset" - what does this variable mean? I cannot parse the meaning
> of its identifer.
>
I've added a comment in _proretset:
# Returns the value of pg_catalog.pg_proc.proretset for the function.
# "proretset" is short for procedure returns set.
# If 1, the function returns multiple rows, or zero rows.
# If 0, the function always returns exactly one row.
>
> Otherwise, looks fine.
>
Thanks for great feedback! I like your attention to detail!
>
> Regards,
>
> Shlomi Fish
>
> --
> -----------------------------------------------------------------
> Shlomi Fish http://www.shlomifish.org/
> Best Introductory Programming Language - http://shlom.in/intro-lang
>
> Larry Wall has been changing the world. By modifying its very source code.
>
> Please reply to list if it's a mailing list post - http://shlom.in/reply .
>
