Re: Mail::Transport::IMAP4 bug

--hHWLQfXTYDoKhP50
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

* Raul Dias ([email protected]) [050525 20:10]:
> On Wed, 2005-05-25 at 10:59 +0200, Mark Overmeer wrote:
> > * Raul Dias ([email protected]) [050525 06:41]:
> > I have quite bad experiences in my attempts getting bugs in IMAPClient
> > fixed.  Hope you get better results.  Or maybe someone can take over
> > its maintenance...  I have quite a long list of improvements...
> > The code hasn't been updated in 18 months.
> 
> can you post this fixes?

Attached.  Its a lot, but when I was judging the module to take, it was
the best on CPAN.  I have taken a few orpan modules into my custody,
but currently more then enough on my hands.... 

> Also I think that a patchs dir in the Mail-Box package with all patchs
> is a good way to distribute the imprvements to Mail::Box users.

That is not the way to do it... the modules should get patched.

> >   The (expected) performance ...
> This would be another good adition to the docs :)

It's in some of the PDF papers.  It's not that easy to find a good spot
for it in the manual pages.  Patches welcome ;-)

MailBox has already >60.000 lines of doc, but there is always need for
more.
-- 
               MarkOv

------------------------------------------------------------------------
drs Mark A.C.J. Overmeer                                MARKOV Solutions
       [email protected]                          [email protected]
http://Mark.Overmeer.net                   http://solutions.overmeer.net

--hHWLQfXTYDoKhP50
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=IMAPClient-comments


Dear David,

My MailBox module is a full e-mail handling suite, which contains over
140 classes already.  I do not know if you ever had the time to look
into it, but you may get the best overview by looking at the main
index at
  http://perl.overmeer.net/mailbox/html/Mail::Box-Index/

One of my main targets it to hide all differences between folder types,
so the user can write code without type dependency.  This has been
done with success for Mbox, MH, Maildir, and POP3 folders, and I have
to get IMAP4 to work before Dec 5, the deadline of my project.

Your large IMAPClient module has all the features I need to get this
task done, but the task is still quite complicated.  For instance, I
want the login-method to be transparant (provide a default "AUTO"
method which tries all).
For POP3, the connection can be lost during folder access, and it will
be automatically rebuilt without the user application noticing, I would
like the same for IMAP4. Etc.

Anyway, I may contact you a few times in the coming weeks with some
questions or remarks.  I hope you have time/joy to answering them,
but feel free to ignore them or tell me to get lost ;-)

After reading all the docs twice, and scanning most of the code, I have
the first bunch of remarks.  It is about version 2.2.8, which is for
some reason still the default for CPAN.pm to install, but not the
latest...

* Comments on the docs

Sometimes it is hard to figure-out the possibities to achieve a task, for
instance to change flags or to search... This is mainly caused by the
enormous amount of methods which are sorted alphabetically, not on
functionality.

I have the same problem: my main problem has over 900 documented methods.
Therefore, I have created OODoc which helps you group methods in sections.
In your case, it could be helpful to separate the search and the flag-change
methods from the other methods...

* Some typos in the docs

 - Somewhere in an example:  Passord

 - Your copyright notices do not list 2003.

* Code Remarks

I like to go through code when using a module, and yours is pretty good
readible. Still, as former Perl teacher, I would like to nag with some
things you may change.  Please do not feel offended by my remarks, and
ignore anything you don't like.

 - You often use

     sub unset_flag {
        my($self, $flag, @msgs) = @_;
        if ( ref($msgs[0]) =~ /ARRAY/ ) { @msgs = @{$msgs[0]} };

   pattern matching is much slower than string comparison, and in this
   case even dangerous -- my $msg is an object which is called MARRAY
   Faster and safer is to use  ref $msgs[0] eq 'ARRAY'

   My notation for the same method start is a little more straight
   forward

     sub unset_flag {
        my ($self, $flag) = (shift, shift);
        my @msgs = ref $_[0] eq 'ARRAY' ? @{$_[0]} : @_;

 - Methods like

     sub unmark {
        my($self, @msgs) = @_;
        if ( ref($msgs[0]) =~ /ARRAY/ ) { @msgs = @{$msgs[0]} };
        $self->unset_flag('\\Flagged', @msgs);
     }

   can be written much simpler:

     sub unmark {
        my $self = shift;
        $self->unset_flag( '\\Flagged' => @_ );
     }

 - Do I understand this correctly?

     sub size {
        my ($self,$msg) = @_;
        # return undef unless fetch is successful
        my @data = $self->fetch($msg,"(RFC822.SIZE)");
        return undef unless defined($data[0]);
        my($size) = grep(/RFC822\.SIZE/,@data);

        $size =~ /RFC822\.SIZE\s+(\d+)/;

        return $1;
     }

   could be written as

     sub size {
        my ($self,$msg) = @_;

        my @data = $self->fetch($msg,"(RFC822.SIZE)");
        return undef unless defined $data[0];

        /RFC822\.SIZE\s+(\d+)/ && return $1
             foreach @data;
     }

  Saves a match.

 - I saw

  sub status {
        ...
        my @pieces = @_;
        $self->_imap_command("STATUS $box (". (join(" ",@_)||'MESSAGES'). ")") or return undef;
  }

   @pieces is not used.
   You may also write:

        my $what = @_ ? join(" ", @_) : 'MESSAGES';
        $self->_imap_command("STATUS $box ($what)") or return undef;

 - Slow, unsafe and against OO principles:

           } elsif ( ref($msgspec) =~ /MessageSet/ ) {

   should become

           } elsif ( $msgspec->isa('Mail::IMAPClient::MessageSet') ) {

 - The same for

      sub Range {
        require "Mail/IMAPClient/MessageSet.pm";
        my $self = shift;
        my $targ = $_[0];
        #print "Arg is ",ref($targ),"\n";
        if (@_ == 1 and ref($targ) =~ /Mail::IMAPClient::MessageSet/ ) {
                return $targ;
        }
        my $range = Mail::IMAPClient::MessageSet->new(@_);
        #print "Returning $range :",ref($range)," == $range\n";
        return $range;
     }

  which could become:

     sub Range {
        my $self = shift;
        eval "require Mail::IMAPClient::MessageSet";
        die $@ if $@;

          @_==1 && ref $_[0] && $_[0]->isa('Mail::IMAPClient::MessageSet')
        ? $_[0]
        : Mail::IMAPClient::MessageSet->new(@_);
     }

  But probably you wish Mail::IMAPClient::MessageSet to be extendible, so
  not hard-coded....  This requires a new option to ->new() to specify
  the type to be created.

* Enhancement requests

 - I was programming with flags today, so that's why most of my remarks
   are in that direction.  Looking at

      sub unset_flag {
        my($self, $flag, @msgs) = @_;
        if ( ref($msgs[0]) =~ /ARRAY/ ) { @msgs = @{$msgs[0]} };
        $flag =~ /^\\/ or $flag = "\\" . $flag
                if $flag =~ /^(Answered|Flagged|Deleted|Seen|Draft)$/i;
        $self->store( join(",",@msgs), "-FLAGS.SILENT (" . $flag . ")" );
      }

     sub unmark {
        my($self, @msgs) = @_;
        if ( ref($msgs[0]) =~ /ARRAY/ ) { @msgs = @{$msgs[0]} };
        $self->unset_flag('\\Flagged', @msgs);
     }

   I wonder why only one flag can be used at a time.  In my case, I often
   move messages from a non-imap folder into imap, and then need to set
   a whole bunch of flags at the same time.  Besides, all the macro's
   you make have the flag already in correct format, so why check it
   again?

   Can I suggest the following:

     sub _unset_flags {
        my ($self, $flags) = (shift, shift);
        local $" = ',';
        $self->store( "@_ -FLAGS.SILENT ($flags)" );
     }
        
     sub unset_flag {
        my $self  = shift;
        my @flags = ref $_[0] eq 'ARRAY' ? @{ (shift) } : shift;

        s/^(Answered|Flagged|Deleted|Seen|Draft)$/\\$1/i for @flags;
        $self->_unset_flags( join(' ', @flags), @_);
     }

     sub unmark {
        my $self = shift;
        $self->_unset_flags( '\\Flagged' => @_ );
     }

 - Accessors are called very often.  Pleople always complain that
   calling them is so expensive, so some even access the object's
   hash to avoid the minor performance hit.

   What I do not understand is why your _do_accessor is so complex.
   For each accessor which is called, you perform check to see whether
   the accessor has some extra needs or not.  Methods which do other
   things than simply get and/or set values are no accessors.

   So... I suggest to provide explicit methods to Fast_io, Socket, and
   LastError to simplify the code for the other accessors.

--hHWLQfXTYDoKhP50--