Repository navigation
fix(gh-446): accept the motherduck: prefix in the city renderer and sync - #457
JesuFemi-O wants to merge 1 commit into
Conversation
PR title checkmode:
Result: ✅ title matches the convention. |
PR size checkmode: Files changed: 8 counted / 8 allowed
Result: ✅ within limits. |
8dfc848 to
2dd8994
Compare
75b251f to
e0c59d6
Compare
There was a problem hiding this comment.
Independent review at e0c59d6. This replaces an earlier automated approval whose summary was not based on the diff. The approval stays, but the item under "Should fix" is worth settling before this merges.
What it does: tycoon city (_serve_target) and tycoon data sync use is_motherduck() from tycoon.project. Because tycoon_city can't import tycoon, catalog/loader.py gets its own two-prefix is_motherduck, used by pricing.py and webserve.py, and the catalog-name logic strips either prefix. It also rewrites a test that used to open a real MotherDuck connection without a token.
Checked: motherduck:dogfood gets the same alias, name and serve target as md:dogfood; motherduck:_share/n/uuid names the catalog n; local catalogs are unaffected. All 11 CI checks pass.
Should fix
src/tycoon_city/catalog/tycoon_project.py:62: the defaulttycoon city, which reads the project directory, still buildswarehouse_path = root / warehouse, somotherduck:mydb(andmd:mydb) fails with "Database file not found". The body saysCloses #308, but #308 is still live on this path. Fix it here, or change toRefs #308and track it separately.
Nits
loader.py_database_name_forandsync.py_alias_forkeep a?motherduck_token=query in the display name and the ATTACH alias, and the name reachescity.json. Splitting on?first is cheap since this PR touches both lines. This predates the PR but now applies tomotherduck:too.is_motherduckis now public inloader.pybut not in__all__.
tycoon city and sync use the helper from tycoon.project. tycoon_city does not import tycoon (docs/city/conventions.md), so the loader carries the same two-prefix rule and pricing and webserve import it from there. The catalog name logic strips either prefix.
e0c59d6 to
69282ea
Compare
What this does
#454 taught the
tycoonpackage to acceptmotherduck:as well asmd:. Four places still checkedstartswith("md:"), so amotherduck:catalog was resolved as a local path there: the city renderer and its catalog loader (src/tycoon_city/),tycoon city, andtycoon data sync. They now accept both prefixes.Mental model
motherduck:my_dbpassed totycoon city, the city renderer ortycoon data sync.tycoon cityandsyncuse theis_motherduck()helper fromtycoon.project.tycoon_citymust not importtycoon(docs/city/conventions.md), socatalog/loader.pycarries the same two-prefix rule andpricing.pyandwebserve.pyimport it from there. The catalog-name logic strips either prefix, and the check is case-insensitive like DuckDB's (MD:my_db,MotherDuck:my_db);./MD:my_dbstays a file path.motherduck:xandmd:xbehave the same. A local path or anmd:string behaves as before.Known gap
The default
tycoon citypath, which serves the project directory, still fails on a MotherDuck warehouse with "Database file not found":src/tycoon_city/catalog/tycoon_project.pybuildsroot / warehouseas a path. This PR already touches 8 files, so fixing it here would exceed the cap and it is left as a known remaining gap tracked by #446.tycoon city --path md:dbpasses the target through to the server unresolved, which a test covers; I did not run it end to end against a real catalog.Linked issue
Refs #446
Refs #308
Depends on #456. This is the last PR for #308.
Type of change
Checklist
pre-commit run --all-filespassescsv-importpipeline still runs end to end (this is the CI gate — if it breaks, the release is blocked)CHANGELOG.mdif this is user-facingNotes for reviewers
This change does not alter layout and touches no file under
web/, so there is no screenshot. The catalog-name tests intests/tycoon_city/catalog/test_loader.pyandtests/test_city_command.pyshow the behaviour instead.I rewrote
test_motherduck_source_is_not_treated_as_a_file_path. It calledload_catalog("md:definitely_not_a_real_catalog_xyz")with nothing mocked, so it opened a real MotherDuck connection. With no token set, DuckDB answers that with a browser sign-in, so every defaultpytestrun on a machine without a token could pop up a device confirmation. The test now mocksduckdb.connectand runs for both prefixes.This PR and #454 each sit at the 8-file cap.
Verification:
uv run pytest -qat this branch gave 1666 passed and 10 skipped, withMOTHERDUCK_TOKEN=invalid-token-for-testandDLT_DATA_DIRset to a writable directory.load_catalogand thedata syncATTACHworked under both prefixes against a scratch database. That script is not committed as a test.md:strings and local paths resolve as before, including paths that containmd:later in the string.pre-commit run --all-filesfails on a clean tree and rewrites nine unrelated files, so I ranpre-commit run --fileson the changed files instead, plusruff checkandruff format --check.