Fix: Initializing sqlite with `architect` causes PartitionTypeError

[alph486]

Hi there,

In 0.2.0, when experimenting with the new sqlite dummy support, I noticed an issue. I implemented simple Meta Fields in my Model I want to partition.

class Event(PartitionableMixin, models.Model):

    class PartitionableMeta:
        partition_type = 'range'
        partition_subtype = 'date'
        partition_range = 'month'
        partition_column = 'timestamp'
...

This works fine in when using Postgres (which is in my server environment). When testing locally using Django 1.7.1 and Sqlite3, architect partition --module my.app.models would produce:

Unsupported partition type "range" in model "Event", supported types for "sqlite" database are: range

I traced this back to cursor.connection.autocommit apparently not being available with this backend. Apparently, isolation_level = None will do this according to the docs . After making the attached changes it appears to work correctly.

I'm not sure how you feel about passing the dialectinto the Database __init__, but it was quick. I will also admit I am a novice with travis and nose, but I attempted to add tests for SQLite+Django to just verify it could call the command without breaking and insert data.

Please let me know your thoughts.

Thanks Kevin

[coveralls]

Coverage increased (+0.03%) when pulling 76d333c813505de7cf83dd581d9c5de0d0017a78 on alph486:master into efe078cc9187109b3d02777adabbbf036e856fa1 on maxtepkeev:master.

[alph486]

Oops, realized the if-statement logic was a bit awkward. New revision.

[coveralls]

Coverage decreased (-14.71%) when pulling 63277455582cf883655d9a57837453d3aef85d6d on alph486:master into efe078cc9187109b3d02777adabbbf036e856fa1 on maxtepkeev:master.

[coveralls]

Coverage increased (+0.54%) when pulling 63277455582cf883655d9a57837453d3aef85d6d on alph486:master into efe078cc9187109b3d02777adabbbf036e856fa1 on maxtepkeev:master.

[maxtepkeev]

Hi,

Good job.

Though as you said I don't feel it's a good idea to pass the dialect into the Database __init__. Also setting autocommit to True breaks Django's transaction atomic (see #4), so the __init__ needs to be rewritten anyway.

I'll think about how this can be done to not to break everything.

Thanks for findings and tests.

[alph486]

Thanks! I can understand that. If transaction atomic is already broken though don't these changes only improve existing functionality? Not that a better solution shouldn't be reached, but its a decent stopgap no?

[maxtepkeev]

You're absolutely right, the problem is I don't like this kind of half-solutions. I'm not saying that they're bad or something, it's just me and my way of thinking.

I believe that we should try to create proper solutions from the first time and only such code should be merged into master, because from my practice it always turns out that if you put some not very elegant but working code into master, than you forget about it because it just works and concentrate on other problems ;-)

[alph486]

Understood, let me know if you figure anything out. Thanks!

On Thu, Nov 27, 2014 at 1:03 AM, Max Tepkeev notifications@github.com wrote:

You're absolutely right, the problem is I don't like this kind of half-solutions. I'm not saying that they're bad or something, it's just me and my way of thinking.

I believe that we should try to create proper solutions from the first time and only such code should be merged into master, because from my practice it always turns out that if you put some not very elegant but working code into master, than you forget about it because it just works and concentrate on other problems ;-)

— Reply to this email directly or view it on GitHub https://github.com/maxtepkeev/architect/pull/7#issuecomment-64753126.

Kevin J. Kuhl

[maxtepkeev]

Fixed in v0.3.0. Please see docs, because Architect was almost completely rewritten.