Moving database connection stuff to a dedicated db module #757
Conversation
|
| db_session = scoped_session(sessionmaker(autocommit=False, | ||
| autoflush=False, | ||
| bind=engine)) | ||
| from db import db_session, engine |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
In spirit this seems fine. But do we want to be adding other tables and data to the "session.db"? Or do we want to create new databases for different things, i.e. issues? I don't actually have an answer, curious what others think. (If we keep it all in a single db, we should probably rename it to not be session.db) |
|
I have a hunch that it should be a separate db, for at least one reason, that it is easier to un-mess things if we need to fully drop it because it has been polluted. On the other hand, we probably want to keep the bugs data if any in the future. |
|
I also tend to like the idea of separate databases. And right now I can't think of any relationships between user_id/session and a table of issues... On the other hand, at some point we will probably want a relationship between issues and usernames for dashboard stuff. But we don't store usernames in our session table. @hallvors can you refactor this so we're creating two databases, one for session and one for issues? |
|
@karlcow This is a little refactoring that is meant to make the fix for #165 easier and cleaner. Is it mandatory to have an Issue and a Pull Request even for small fixes? IMHO a PR is a good place to discuss things ;) @miketaylr Sure, will change this to have two data files. I also forgot to do the pep8 thing.. |
Probably not mandatory, usually when I do this kind of refactoring I'm already inside a branch working on a feature (that has an issue). So it would just be a single commit like I would say for simple fixes a PR is fine, but for stuff that would benefit from discussion Issues are a good thing™ (even though yes there are comment boxes in this PR :P). |
|
I've fixed the variable names to be session_db and session_engine, so whenever other parts import those it should be pretty clear what DB we're using. When we need other files and tables we can add them in |
|
Hm.. not sure why the Travis build fails now, this runs OK locally as far as I can tell. |
It looks like some unrelated label test failures. Somehow a label got left on the test issue, so everything that expects it to not have a label fails. Those tests have been flaky in the past, so I wouldn't worry about it. Hopefully once #712 lands, we'll be less affected by these kinds of failures. |
Looks good to me. In a perfect world the commit messages would start with (Also that way they are cross-linked in the issue and make it easier to track down the commits/history in the CHANGELOG (we list PR and Issue)). But I'll take the contribution over perfection. ^_^ |
Issue #165. Moving database connection stuff to a dedicated db module
|
I deployed this to staging and logging in and out still works, so that's good. 🎈 |
I propose we collect the code to create a db session in one place, for example like this.
(This may not be The Best Way To Do It - would be good with a review from for example @karlcow )