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

Major database rejig #37

Merged
merged 4 commits into from
Nov 22, 2013
Merged

Conversation

indivisible-irl
Copy link
Member

Minor changes

to mail.py and projects.py - just tacked on as I forgot to push and too lazy to separate them...

=0p

The real work here has been done to

Database handling - db.py.

  • All desired tables should be defined in the dict at the top self.all_tables. Name, Version number (!! increment on schema changes) and SQL create table info - just the column descriptions [ie without CREATE TABLE... etc].
  • Any table defined will be created if not exists (current functionality - in future will be able to test version and upgrade tables self.__on_upgrade() automatically)
  • To skip the managing of a table add it to self.unimplemented_tables and it will be assumed to be ok. It's up to you to manage.

Makes some headway towards Issue #35.

@projectdelphai
Copy link
Member

For lines 257-262, you check whether table exists so that you can drop it and and then continue. So you do two db calls. However the db_drop_table method's SQL is "drop if exists <table_name>" so you should be able to just call the drop_table method and move straight to add table. And I'm not sure why you need go_for_new since there's no way that go_for_new will ever not be True.

And what exactly would be your upgrade strategy? I have no problem with it, I just don't understand it. is the process something like this?

  1. Check version number
  2. if same, do nothing
  3. if different, duplicate table
  4. drop and create new table
  5. and import old_table info

If so, how do you know how to fill in the new data? Seems like this would need to be a per-case thing done manually.

Other than that, I'm perfectly fine with it. Great job on the error catching.

@indivisible-irl
Copy link
Member Author

Ok - in order:

self.__recreate_table(): Yeah, there are two calls to test if the db is there. I'm not entirely confident in the concurrency of the db's methods and people's interactions so even after I realised there was redundancy I left it in for safety. Maybe unnecessary but I can't imagine one extra call for a rarely used method will have any impact.

go_for_new (horrible var name!) will be false only if the database is found but didn't delete/drop successfully. Any other case and it will be true allowing for safe creation of the new table.

The upgrade strategy would be a per table and per version strategy. We will know the old format and new formats of the table and can map the fields, drop unwanted info, update values etc based solely on the requirements for say moving from logs v1 to logs v3.

Yeah, something alone the lines of copy table, drop original, create new table (with original name) perform repopulation of new table with old info applying whatever changes need to be applied on the way if needed.
If table doesn't yet exist (will be at version 0) it's a simple create new table.

indivisible-irl added a commit that referenced this pull request Nov 22, 2013
Major database rejig,

Ok, if there are no objections I'll merge. We can revisit the redundant table exists checking later but as I said - the updates will be called rarely (and now that I think about it likely before the bot even connects to a server).
@indivisible-irl indivisible-irl merged commit 0138e23 into Progether:master Nov 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants