Actions/builders converted to running via config & manifest #256
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
dogversioning
force-pushed
the
mg/config_manifest
branch
from
28e82dc to
701db90
Compare
| if args.get("db_type") == "duckdb": | ||
| schema = "main" |
There was a problem hiding this comment.
This feels like a little bit of duckdb specific logic escaping from database.py, but it's not upsetting.
There was a problem hiding this comment.
oh thank you, i was like 'i need to mention something' and couldn't remember what it was.
If we want to get this cat back in the bag, we'll need to undo the overloading we're currently doing, where raw schema name and duckdb file path are going into the same location. So we'd basically add one new CLI arg and then split how that logic is handled in some places.
if we want to do this, i can create a ticket for it, but i don't want to do it on this PR given the density.
There was a problem hiding this comment.
Hmm, sounds like you are talking a bigger change than I was - I'm not 100% on what your vision there is.
I was talking more like the DuckDB database backend simply ignoring the input schema arg and using main or a method like DatabaseBackend.decide_schema_name(args) or something - just anything to put this tiny business logic under the umbrella of the DuckDB-specific code. Ideally in my mind, nothing outside of databases.py every has to know DuckDB exists.
There was a problem hiding this comment.
Per offline discussion, leaving this as is for now, but logging a ticket: #258
| "SELECT schema_name FROM information_schema.schemata" | ||
| ).fetchall() | ||
| if (schema_name,) not in schemas: | ||
| self.connection.sql(f"CREATE SCHEMA {schema_name}") |
There was a problem hiding this comment.
Rude for SQL to not have CREATE SCHEMA IF NOT EXISTS
dogversioning
force-pushed
the
mg/config_manifest
branch
from
03cabbf to
04b4b79
Compare
dogversioning
force-pushed
the
mg/config_manifest
branch
from
cd533f0 to
06633e1
Compare
This PR makes the following changes:
Checklist
docs/) needs to be updated