Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
gouthambs opened this issue Jul 21, 2015 · 10 comments
Closed

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

gouthambs opened this issue Jul 21, 2015 · 10 comments
Assignees
Milestone

Comments

@gouthambs
Copy link
Contributor

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
Copy link
Owner

ashcrow commented Jul 21, 2015

@gouthambs Absolutely!

@ashcrow ashcrow added this to the 1.1.0 milestone Jul 21, 2015
@gouthambs
Copy link
Contributor Author

@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
Copy link
Owner

ashcrow commented Jul 21, 2015

@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
Copy link
Contributor Author

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

@ashcrow
Copy link
Owner

ashcrow commented Jul 21, 2015

@gouthambs 👍

@ashcrow
Copy link
Owner

ashcrow commented Jul 22, 2015

@gouthambs does this end up being:

-            meta = sql.MetaData()
+            if db:
+                meta = db.metadata
+            else:
+                meta = sql.MetaData()

@gouthambs
Copy link
Contributor Author

@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
Copy link
Owner

ashcrow commented Jul 22, 2015

@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
Copy link
Contributor Author

@ashcrow Sure! Sounds good!

@ashcrow ashcrow self-assigned this Jul 22, 2015
@ashcrow
Copy link
Owner

ashcrow commented Jul 22, 2015

Work done in 23e0f8e. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants