Add option to not create database tables #676
Conversation
Simon-Will
force-pushed
the
feature-675-add-option-not-to-create-tables
branch
2 times, most recently
from
43ea4a1 to
39bc0b1
Compare
FlorianK13
left a comment
FlorianK13
left a comment
There was a problem hiding this comment.
Hi @Simon-Will, thanks for your contribution. I have added two comments, maybe you can adapt it that way?
open_mastr/mastr.py
Outdated
|
|
||
| def __init__(self, engine="sqlite", connect_to_translated_db=False) -> None: | ||
| def __init__( | ||
| self, engine="sqlite", connect_to_translated_db=False, ensure_tables=True |
There was a problem hiding this comment.
ensure_tables is not really self-explanatory. Maybe create_empty_tables?
open_mastr/mastr.py
Outdated
| Only for 'sqlite'-type engines. | ||
There was a problem hiding this comment.
A documentation of this new parameter is missing. @Simon-Will Can you add it here? It will than automatically be displayed in most code editors and on our documentation page.
|
Actually, it turned out that what I did here is not enough: There are other places where existing tables are dropped and a new creation is attempted: At least here: It's probably for ensuring that old data is deleted, right? I will see if I find a reasonable way to adapt this. |
Simon-Will
force-pushed
the
feature-675-add-option-not-to-create-tables
branch
2 times, most recently
from
100199f to
bf4f63e
Compare
Simon-Will
force-pushed
the
feature-675-add-option-not-to-create-tables
branch
from
bf4f63e to
0983194
Compare
|
I updated this PR and think that it's ready now. Could you have another look, @FlorianK13? I was a bit surprised to find some ALTER statements quite deep in the code. This meant that I also had to pass along the new switch quite deeply. I think it's alright for now, but do you think it makes sense to concentrate the table creation in one point: After downloading the Gesamtdatenexport, one could quickly scan for columns not present in the SQLAlchemy models. And only then create the tables. When the tables are created, one could start importing. |
|
@Simon-Will The amount of technical debt in this repository is always good for a surprise or two. Regarding the creation at one place - we are somehow touching this topic in our discussion in #663 |
|
@FlorianK13, is there anything else you'd like me to do here? Or is it too deep a change in general? |
|
@Simon-Will I haven't found the time yet to review your PR since I have only limited resources for open-mastr. I'll ping you if I think more changes are necessary. |
|
@FlorianK13, yes, perfectly fine! I didn't want to put pressure on you; was just trying to make sure that you're not currently waiting for anything from my side. Take your time. :) |
|
Hi @Simon-Will - as a short update: @nesnoj plans to make a release next week, but we have not really worked on your PR yet. So your PR will very likely not be part of the release next week. Since we have more PRs waiting to be merged that will not make it into the release next week, we hopefully manage to have another release soon. |
|
I really hope this is not too demotivating for you :/ It is just that me and the other maintainers really struggle with their time budget and (sadly) open-mastr is not always first priority. |
|
@FlorianK13, thanks for the heads-up! That's understandable and quite alright. See you at the workshop at EWI next week in case you're there! |
|
No, I won't make it - but I wish you a nice time at the workshop (: |
|
Converted to draft, since the work in #516 could make this PR to be irrelevant |
2 participants
Summary of the discussion
I propose to introduce this little option for
Mastrthat enables the user to prevent it from creating tables automatically. If you agree this is a sensible addition, I can also add some documentation for it.Type of change (CHANGELOG.md)
Added
ensure_tableswith default valueTrueto classMastrthat controls whether to create database tablesAutomation
Closes #675
PR-Assignee
Reviewer