ashcrow / flask-track-usage

Basic metrics tracking for the Flask framework.
Other
53 stars 33 forks source link

Use the flask-sqlalchemy metadata instead of creating a new one #22

Closed gouthambs closed 9 years ago

gouthambs commented 9 years ago

I found a small issue with the way the MetaData is constructed in the sql.py. Would like to fix this if you are still taking pull requests!

ashcrow commented 9 years ago

@gouthambs Absolutely!

gouthambs commented 9 years ago

@ashcrow Here is some background for this issue. While working on the flask-blogging extension, I have a storage and the main engine similar to the flask-track-usage. I found that the storage tables were not recognized in the database migrations while using Alembic automigrate feature. It looks like the reason for that is the MetaData object should all be unified. The Flask-SQLAlchemy SQLAlchemy object has a metadata internally, and we can directly use that instead.

ashcrow commented 9 years ago

@gouthambs Ah, I see. Are you looking at allowing metadata to be passed into the set_up() so it can be reused if given (similar to the db=None input)?

gouthambs commented 9 years ago

Actually the db object has the metadata property. So we wont need to add any more variables.

ashcrow commented 9 years ago

@gouthambs :+1:

ashcrow commented 9 years ago

@gouthambs does this end up being:

-            meta = sql.MetaData()
+            if db:
+                meta = db.metadata
+            else:
+                meta = sql.MetaData()
gouthambs commented 9 years ago

@ashcrow Thats correct.

Another thing is I feel the tables should not be created by this storage class. Explicit invocation of db.create_all() should take care of that. Thats a small change in usage.

This is consistent with how Flask-SQLAlchemy models just create the table instances in the meta, but the explicit invocation of db.create_all() creates the tables.

ashcrow commented 9 years ago

@gouthambs I'll commit the above patch. Let's discuss explicit db.create_all() rather than creating the tables automatically if they do not exists in a different issue to keep the work separate.

gouthambs commented 9 years ago

@ashcrow Sure! Sounds good!

ashcrow commented 9 years ago

Work done in https://github.com/ashcrow/flask-track-usage/commit/23e0f8ef4e6647a0b873882ee184a7b7fa4f20fb. Closing this issue.