[PATCH] Re: introducing xattrMaildir: a Maildir repository where message files have all IMAP flags in an extended attribute

[Nicolas Sebrecht]

On Tue, Mar 12, 2013 at 10:54:01AM +0100, Erik Quaeghebeur wrote:
> Dear Nicolas,
> 
> First of all, I must stress that this patch cannot work without my 
> earlier one “Let OfflineIMAP sync all flags between IMAP servers, not 
> just Maildir-compatible flags”, which is far more invasive, but—I 
> think—also useful to a much wider audience; formulated differently: I 
> think it corrects a historically understandable but wrong design choice 
> and is the more important of the two, needing close scrutiny.

Could you elaborate about this "wrong design choice", please? I don't see what
you're talking about and how you enhance it.

> Ok, I'll try to do that when preparing a new patch that modifies Maildir 
> to take an xattr option instead of adding a separate repository type.

<...>

> Um, not knowingly, but xattrMaildir does essentially just derive from 
> Maildir and then applies some changes to a few functions. The class 
> __init__ functions are not modified but inherited.

<...>

> I avoided refactoring to not make the patch too big and the deviation 
> from the current code as small as possible.

Here is a detailed explanation as I was not clear enough. What I'm
suggesting is the exact oposite: refactoring in order to have as small
as possible duplicated code in the inherited object.

Here is a sample. Starting from

A():
  def original_method(arg1, arg2):
    statement1(arg1)
    statement2(arg2)
    statement3
    return result


I suggest something like this:

A():
  #
  # Refactored new "private/hidden" method (might have a better name!).
  #
  def _original_method(arg1, arg2, new_arg3=optionnal):
    # Don't focus of the following statements, it's out of concern.
    statement1(arg1)
    statement2(arg2)
    if new_arg3:
      statement4  # refactored piece has to honor the new_arg3 argument
    else:
      statement3  # does not differs from original behaviour
    return result

  #
  # Keep the object interface intact.
  #
  def original_method(arg1, arg2):
    return _original_method(arg1, arg2, False)


NewInheritedB(A):
  #
  # Keep the object interface intact.
  #
  def original_method(arg1, arg2)
    #
    # Last argument could also be a variable coming from a
    # original_method() optional argument.
    #
    return _original_method(arg1, arg2, whatever_third_arg)


> > I think the new repository type approach is good because of the library
> > dependency issue.
> 
> Would it be ok to add the functionality to Maildir if the extra 
> dependency is hidden behind an option-controlled dependency,

Not sure what you mean by "option-controlled dependency".  You already
have the option for that: the maildir type you just introduced.

If you mean to have the import statement womewhere inside the original
method itself: this is crappy.  And yes, I know we have such thing in
offlineimap/threadutil.py:161.

Back to my above sample, you could do something like this (with a
comment on top of the file where imports are expected):

NewInheritedB(A):
  #
  # Keep the object interface intact.
  #
  def original_method(arg1, arg2)
    try:
      import BLAH
      #
      # Last argument could also be a variable coming from a
      # original_method() optional argument.
      #
      return _original_method(arg1, arg2, whatever_third_arg)
    except ImportError:
      # Handle missing dependency


>                                                              as i had 
> the impression you suggested above> (I now think this is the cleanest 
> way to add the functionality, given that xattrMaildir differs so little 
> from Maildir.)

Not a problem to use polymorphism (aka a dedicated object) for that
purpose. The other good thing I see with this approach is to have a
dedicated-inherited object for a /not usual/ OfflineIMAP repository
type.

Now, I don't have strong opinion on this as long as the import thing
does not need to be in a random place somewhere in the code.

Thanks!

-- 
Nicolas Sebrecht