[bop-devel] Possible changes to Business-OnlinePayment 3.x, comments anyone?

[Phil Lobbes]

Below are some changes I was considering making (if there are no major
objections) to Business-OnlinePayment 3.x.  I'm throwing this out to the
group incase anyone is particularly for or against any of them and to
keep everyone in the loop.  Several of these are trivial changes but a
few may be of more general interest.  Any suggestions, comments, and
criticisms are welcome too :-).

Phil

-----------------------------------------------------------------
1. I'd like to remove these commented out things:

	# @ISA @EXPORT @EXPORT_OK $AUTOLOAD);
	#use Data::Dumper;
	#require Exporter;
	#@ISA = (); #qw(Exporter AutoLoader);
	#@EXPORT = qw();
	#@EXPORT_OK = qw();

-----------------------------------------------------------------
2. I'd like to replace this:

  $VERSION = '3.00_04';
  sub VERSION { #Argument "3.00_01" isn't numeric in subroutine entry
    local($^W)=0;
    UNIVERSAL::VERSION(@_);
  }  

With:

  $VERSION = '3.00_04';
  $VERSION = eval $VERSION;

  [ excerpt from perlmodstyle ] 

  If you want to release a 'beta' or 'alpha' version of a module but
  don't want CPAN.pm to list it as most recent use an '_' after the
  regular version number followed by at least 2 digits,
  eg. 1.20_01. If you do this, the following idiom is recommended:

    $VERSION = "1.12_01";
    $XS_VERSION = $VERSION; # only needed if you have XS code
    $VERSION = eval $VERSION;

  With that trick MakeMaker will only read the first line and thus
  read the underscore, while the perl interpreter will evaluate the
  $VERSION and convert the string into a number. Later operations
  that treat $VERSION as a number will then be able to do so
  without provoking a warning about $VERSION not being a number.

-----------------------------------------------------------------
3. OK if I have 'required_fields' croak with a list of all required
  fields that are missing?

  sub required_fields {
      my($self, at fields) = @_;

      my %content = $self->content();
      foreach(@fields) {
          Carp::croak("missing required field $_") unless exists
          $content{$_};
      }
  }

becomes something like:

  sub required_fields {
      my($self, at fields) = @_;

      my @missing;
      my %content = $self->content();
      foreach(@fields) {
          push(@missing, $_) unless exists $content{$_};
      }

      Carp::croak("missing required field(s): " . join(", ", @missing) . "\n")
        if(@missing);
  }

-----------------------------------------------------------------
4. Do we want to consider having get_fields be backwards compat w/2.x?

  Right now it no longer "keeps" content with undefined values (which it
  did do with 2.x.  I'm not sure if this change was on purpose or not, I
  didn't see any comments about this in the changelog.

  For example:

    map { $_ => $content{$_} } grep defined $content{$_}, @fields;

  would just become:

    map { $_ => $content{$_} } @fields;

  # routine in 3.x (2.x code still in there just commented out)

  sub get_fields {
      my($self, @fields) = @_;

      my %content = $self->content();

      #my %new = ();
      #foreach(@fields) { $new{$_} = $content{$_}; }
      #return %new;
      map { $_ => $content{$_} } grep defined $content{$_}, @fields;
  }

-----------------------------------------------------------------
5. Any interest in having remap_fields() not modify %content?

This would break backwards compatibility so I doubt we want to do this
one, but in general I would prefer if remap_fields didn't replace
%content I noticed that the author of B::OP::InternetSecure didn't like
that either.  In B::OP::PayPal I created a get_remap_fields() methods
that is a little more flexible for something to compare remap_fields()
against.  Any thoughts on this one?

-----------------------------------------------------------------
6. [ completely optional idea ] "fill paragraphs" (wrap text blocks)

  While changing documentation I could quickly "fill paragraphs"
  (i.e. take lines in docs that run longer than ~72 chars and wrap the
  text to keep them from being overly long.  Not a big deal, but easy
  for me to do if you're ok with that.

-----------------------------------------------------------------
7. [ completely optional idea ] perltidy!

  I wouldn't mind running the whole thing through 'perltidy' if you're
  really up for a format change... but I'm fine if we don't do this too
  of course.