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

[Ivan Kohler]

On Tue, Aug 29, 2006 at 09:23:35PM -0400, Phil Lobbes wrote:
> 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 :-).

Comments inline.

> -----------------------------------------------------------------
> 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();

Sure.

> 
> -----------------------------------------------------------------
> 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;

Sure.

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

Yup.

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

I don't think so.

I don't want to break backwards-compatibilty in most cases, but I don't 
think we need to maintain bug-for-bug compatibility.  If this doesn't 
break processing in some real-world way, I"d like to keep the change.

If ability to pass undef was something folks found useful for testing, 
perhaps we could have a flag of some sort to enable it.


> -----------------------------------------------------------------
> 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?

remap_fields should behave as it does now, for backwards compatiblity.  

Create a new method that does what you want, i.e. feel free to move 
get_remap_fields to the B:OP base class.

Similarly I have a "revmap_fields" that works backwards from 
remap_fields - I've copied it around several processor modules but it 
should probably be checked into the base class now also.

> -----------------------------------------------------------------
> 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.

Sure.

> -----------------------------------------------------------------
> 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.

I'm slightly opposed as I think this is unnecessary; however if everyone 
else would prefer it, I'll save my karma for arguing about things I care 
more about.  :)

-- 
_ivan