[PATCH 3/9] Re: Experimental LocalStatus stored in SQLite database

[Sebastian Spaeth]

On Wed, 27 Apr 2011 19:13:08 +0200, Nicolas Sebrecht wrote:
> On Tue, Apr 26, 2011 at 12:31:34PM +0200, Sebastian Spaeth wrote:
> > 
> > Based on patches by Stewart Smith, updated by Rob Browning.
> > This adds the file without using it yet.

> Don't we want PEP8 (one import per line)? I remember you sending patches
> like this. :-)

Right, this basically adds the file as was provided without major
modifications. I cleaned it up in the next patch, but we should probably
be doing that here already.
 
> > +
> 
> Why a blank line?

Not sure :)
 
> > +	self.dbfilename = self.filename + '.sqlite'
> 
> Is it the right place for this statement? No newline before?

I think it is the right name, we initialize the name where we expect to
find/put the db filename in __init__, but any place is good as long as
we set it somewhere :)
 
> > +
> > +	# MIGRATE
> 
> I don't think we want such migration plans here. Handling database
> format here could end with lot of migration code we don't usually care
> about.

This is a leftover from the original. I have done away with all
migration code in the next patch, I think. The reason is that we can
just recreate the LocalStatus information from scratch if both LOCAL
and REMOTE are reasonably in sync. So, we can just disregard the old
plain text status and recreate the new one as if the LocalStatus cache
were just missing.

> Say we're at v6.3.3:1 (:1 for the database format release). Then, comes
> v6.4.0:2 and later v6.5.0:3... Yeah, you got my point: what shoud we do
> for people going from :1 to :3?  And we will get things worse than that
> having SQL optionnal at the Account level.
 
> Migrate from LocalStatus to LocalStatusSQLite _IS_ a migration like
> above. A bit particular due to the creation of the database but it's
> really the same.

mmh, as I've written above. If both repos are reasonably in sync, we can
just recreate LocalStatus without any special need for migration. We do
store a "version" number of the table format, so we can upgrade people
that are on old versions when we bump it.
> 
> Indentation looks ugly.

Thanks will have a look
 
> > +	        file = open(self.filename, "rt")
> > +	        self.messagelist = {}
> 
> Repeated statement?

Will check
 
> > +	        line = file.readline().strip()
> > +	        assert(line == magicline)
> 
> I would add a comment here or implement a dummy (no-op) migration
> process depending on the migration implementation.

Right

> > +	        for line in file.xreadlines():
...
> > +	        file.close()
> 
> Indentation mistakes?

I think it is correct, but will check.
 
> > +	# create new
> 
> Notice: a creation is a migration. :-)
:)
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/offlineimap-project/attachments/20110428/336bc1bb/attachment.pgp>