[Bioperl-l] Inconsitency in return value ofBio::SimpleAlinment::remove_columns()

[Seiji Kumagai]

On Fri, 22 Sep 2006, Chris Fields wrote:

>> I, at the same time, noticed another issue with the method. According to
>> the POD, the method returns a new alignment. However, iff argument is not
>> passed to it, the return value is $self. I see this may be a problem
>> because modifications on the returned value affect the original object
>> only if this method is called in the way I mentioned. Secondly, it is just
>> confusing to me that returned value is different but superficially
>> identical (both are Bio::SimpleAlignment object).
>>
>> I want to make a modification on this issue by one of the following ways:
>>
>> 1) Make an argument mandatory. If argument is missing, throw an
>> exception. It is user's responsibility to handle it properly.
>>
>> 2) Always return new SimpleAlign object. If the argument is not passed,
>> return a clone of $self. This may make the scripts run slower and use more
>> memory when no argument is given, but modification on new object is
>> guaranteed not to affect original object.
>>
>> 3) Do not modify current version.
>>
>> 3.a) Do not modify current version, but state the difference in POD.
>>
>> What do you think? I personally think the first is the best since calling
>> the method but doing nothing is not what users want to do in most of the
>> cases. Those rare cases are deserved to be handled by an exception.
>>
>> Thanks,
>
> The first option sounds appropriate considering the use of the method in
> POD.  I also agree that returning $self isn't the best practice either,
> since it would not be a true clone of the object (unless there is some deep
> copy magic going on that I don't see).
>
> What would you suggest?
>

Unless someone says otherwise, I think the following is enough for the 
first option.

Index: SimpleAlign.pm
===================================================================
RCS file: /home/repository/bioperl/bioperl-live/Bio/SimpleAlign.pm,v
retrieving revision 1.110
diff -B -b -u -r1.110 SimpleAlign.pm
--- SimpleAlign.pm      21 Sep 2006 19:20:12 -0000      1.110
+++ SimpleAlign.pm      22 Sep 2006 23:17:23 -0000
@@ -987,7 +987,10 @@

  sub remove_columns {
         my ($self, at args) = @_;
-       @args || return $self;
+       @args || $self->throw
+            (-class => 'Bio::Root::BadParameter',
+             -text => 'Mandatory parameter is missing. see 
documentation',
+             -value 'array reference');
     my $aln;

         if ($args[0][0] =~ /^[a-z_]+$/i) {


Also, I noticed remove_columns() uses or claims to use a coordinate system 
starting from 0, whereas slice() uses a system from 1. Isn't it better to 
have the same scheme?

Thanks,