remote/sqlite-utils
1
0
Fork 0
mirror of https://github.com/simonw/sqlite-utils.git synced 2026-07-23 01:14:31 +02:00
Code Issues Projects Releases Packages Wiki Activity

Remove review notes files

Browse source 
These were working documents for the 4.0 pre-release review - the
actionable findings have all been fixed on this branch and the
changelog and upgrading guide carry the user-facing record.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnLnhsH25Nnv7LHhekUfPd
This commit is contained in:
Claude 2026-07-04 19:56:42 +00:00
commit 509d87b292
No known key found for this signature in database
2 changed files with 0 additions and 542 deletions
Download patch file Download diff file Expand all files Collapse all files

167
create-table-parser-feedback.md
View file

@ -1,167 +0,0 @@
# Review: `create-table-parser` branch
Feedback on the `create-table-parser` branch (commits `ed1de15`, `95d6ff4`,
`407db59` — 819-line `create_table_parser.py` plus JSON test corpora),
reviewed as the proposed foundation for 4.1 `transform()` improvements that
make UNIQUE and CHECK constraints supported instead of silently dropped.
**Verdict: good fit for 4.1 — right design, right release — but it has two
silent-corruption bugs that must be fixed before `transform()` can trust it,
and a few design decisions to settle.**
## Why 4.1 is the right home
- The parser is purely additive: a new module and a new capability. Nothing
about it needs to land in 4.0, so it does not constrain or delay the
stable release.
- The `transform()` improvement it enables — preserving CHECK and UNIQUE
constraints instead of silently dropping them — is a behavior change in
the bug-fix direction, appropriate for a minor release with a prominent
changelog note.
- SQLite exposes no introspection for CHECK constraints (and only partial
detail elsewhere), so parsing the `sqlite_master` DDL is the correct — and
only — approach. The module docstring says exactly this.
## What holds up well (verified empirically)
- **Corpus results:** all 272 statements in `tests/valid_create_table.json`
parse without crashing; the 230 that are independently executable were
cross-checked against SQLite's own `PRAGMA table_xinfo` — **0 column-name
or primary-key mismatches**. The 29 invalid statements also do not crash
the parser.
- Survived adversarial probing: identifier quoting in all four styles
including embedded escaped quotes (`"col ""x"""`, `[square]`, backticks),
strings containing `,)` and keywords inside CHECK expressions,
`col IN (...)` option extraction with escaped quotes, generated columns
(`GENERATED ALWAYS AS ... STORED` and bare `AS`), FK action clauses
(`ON DELETE SET NULL ON UPDATE CASCADE DEFERRABLE INITIALLY DEFERRED`),
`WITHOUT ROWID, STRICT` trailers, keyword-named columns (`"unique"`),
multi-word types with parenthesized arguments, and `CREATE TABLE AS
SELECT`.
- The design is genuinely good: recursive-descent functions mirroring the
SQLite grammar are readable; the constraint list as source of truth with
derived accessor properties (`table.checks`, `col.not_null`,
`table.primary_key`) is the right model — adding a constraint kind means
adding a dataclass and a property. The group-capturing tokenizer
(parenthesized groups taken whole so nested commas never reach the parser)
is a clean way to sidestep expression parsing.
## Must-fix before `transform()` builds on it
### 1. Comments produce phantom columns (silent corruption)
The tokenizer does not handle `--` line comments or `/* */` block comments,
and `sqlite_master` stores DDL verbatim. Reproduced:
```
CREATE TABLE t (
a TEXT, -- user's name, (important)
b INTEGER
)
→ columns parsed as ['a', '-'] (real: ['a', 'b'])
CREATE TABLE t (a TEXT /* legacy, do not use */, b INTEGER)
→ columns parsed as ['a', 'do', 'b'] (real: ['a', 'b'])
```
Hand-written schemas — exactly the ones that have CHECK constraints — are
exactly the ones with comments. If `transform()` rebuilds a table from a
model with wrong columns, that is data loss. Comment handling belongs in
`_tokenize` / `_locate_body` / `_split_top_level` (all three scan raw text).
### 2. Numeric literal defaults are truncated (silent corruption)
`_tokenize` splits `1.5` into `1` / `.` / `5` and `_default_value` consumes
one token. Reproduced:
| DDL | parsed default | correct |
|----------------------|----------------|---------|
| `DEFAULT 1.5` | `'1'` | `1.5` |
| `DEFAULT -1.5` | `'-1'` | `-1.5` |
| `DEFAULT 1e-3` | `'1e'` | `1e-3` |
| `DEFAULT x'0102'` | `'x'` | `x'0102'` |
Re-emitting these in a transform rewrites `DEFAULT 1.5` as `DEFAULT 1` —
silent schema corruption. Notably the corpus already contains
`DEFAULT -45.8e22`, but nothing catches this because the fixtures are
input-only (see next point). Fix by lexing numeric literals (including
sign, decimal point, exponent) and blob literals (`x'...'`) as single
tokens, or by making `_default_value` consume the full literal.
### 3. No tests are actually executed
The JSON corpora are not wired into pytest — there is no `test_*.py` on the
branch, so the parser currently has zero executed tests. Beyond wiring the
corpus in, add **expected-output snapshots** (parse each valid statement,
assert the full structured result), not just "doesn't crash": the
`DEFAULT -45.8e22` bug sat undetected in the corpus precisely because only
inputs are recorded. A cheap high-value addition: property test that for
every executable statement, parsed column names / pk / not-null match
`PRAGMA table_xinfo`.
## Design decisions to settle
### Discard-vs-error policy for grammar the model does not capture
The plan is presumably parse → modify model → serialize, which matches how
`transform()` already rebuilds tables. That makes "what the model discards"
the critical list, because anything discarded is silently stripped from the
user's schema on transform:
- `ON CONFLICT` clauses on PRIMARY KEY / UNIQUE / NOT NULL (parsed by
`_conflict_clause`, thrown away)
- FK `MATCH` and `DEFERRABLE INITIALLY DEFERRED` (parsed, thrown away —
deferred FKs are real in the wild)
- `ASC` / `DESC` in table-level `PRIMARY KEY (a DESC)` / `UNIQUE (...)`
column lists (`_column_list_group` keeps only the leading identifier;
inline single-column PK order *is* captured)
- Unknown column constraints (`_column_constraint` consumes one token and
returns `None`)
Either capture these in the model, or have `transform()` raise
`TransformError` when the source DDL contains grammar it cannot round-trip —
the honest-failure pattern 3.38 established for un-recreatable indexes.
Silence is the one wrong answer.
Same question for column renames: a renamed column referenced inside a CHECK
expression (stored as a raw string) cannot be mechanically rewritten, so
`transform(rename=...)` intersecting a check should raise `TransformError`
rather than emit a stale constraint. Likewise dropping a column referenced
by a table-level CHECK or multi-column UNIQUE.
### Ship it private in 4.1
The module defines `Table`, `Column`, and `ForeignKey` — the package already
has `db.Table` and a `db.ForeignKey` namedtuple with different, incompatible
fields (`(table, column, other_table, other_column)` vs
`(columns, table, references, ...)`). Two public `ForeignKey` types in one
package is a confusion trap, and 4.0 is a fresh reminder that public surface
is forever. Recommendation: land it as a private module
(`sqlite_utils._parser` or similar) powering `transform()` in 4.1, promote
to documented public API in a later minor once battle-tested. Public-later
is additive; public-now-retract-later is breaking.
Related naming nit: the parser's `Table.schema` is the attached-database
name (`main`/`temp`), while `db.Table.schema` is the DDL string. Rename one
(e.g. `database` or `schema_name`) before anything goes public.
### Housekeeping
- `create_table_parser.py` lives at the repo root; it needs to move inside
the `sqlite_utils/` package.
- `parse_checks` is documented as a "backwards-compatible helper" but
nothing in sqlite-utils exists for it to be compatible with — stale
comment from an earlier draft.
- The valid corpus is strong on grammar coverage but light on real-world
mess: add fixtures with comments, tabs/newlines in odd places, numeric
and blob defaults, and a couple of schemas generated by sqlite-utils
itself (double-quoted everything) and by common tools.
## Bottom line
Hold it for 4.1 exactly as planned — it has no 4.0 dependency and should not
delay the stable release. Before `transform()` builds on it: fix comment
handling and numeric/blob literal lexing, wire the fixtures into pytest with
expected-output snapshots, decide the discard-vs-error policy, and keep the
module private for one release. The bones are solid; both bugs are
tokenizer-level and small.

375
fable-review-4.0rc1.md
View file

@ -1,375 +0,0 @@
# sqlite-utils 4.0rc1 pre-release review
Final review before the stable 4.0 release, focused on issues that would be
**breaking changes if fixed after 4.0 ships**. Reviewed at commit `79117b9`
(4.0rc1 plus four commits). Every finding below was verified against the
actual code; the two most serious were reproduced end-to-end.
Test suite at review time: **1080 passed, 16 skipped**.
**Verdict: do not tag stable yet.** There are five release blockers, all of
which are small fixes, plus a handful of semantic decisions that are cheap to
make now and breaking to change later. Recommended path: fix the blockers,
make the documented decisions, cut an **rc2**.
---
## Release blockers
Data loss or wrong-by-default behavior that 4.0 would lock in.
### 1. `delete_where()` never commits and poisons the connection (data loss)
`Table.delete_where()` (`sqlite_utils/db.py:2948`) runs its DELETE via a bare
`self.db.execute()` with no `atomic()` wrapper — compare `Table.delete()` at
`db.py:2944`, which wraps correctly. The connection is left
`in_transaction=True`, so every *subsequent* `atomic()` call takes the
savepoint branch (`db.py:430-440`) and never commits either.
Reproduced end-to-end:
```python
db = sqlite_utils.Database("dw.db")
db["t"].insert_all([{"id": i} for i in range(3)], pk="id")
db["t"].delete_where("id = ?", [0]) # conn.in_transaction is now True
db["t"].insert({"id": 50})
db["u"].insert({"a": 1})
db.close()
# Reopen: rows are [0, 1, 2] — the delete, row 50, AND table u are all gone.
```
In 3.x this leak was latent because later operations used `with db.conn:`
which committed the pending work (verified against 3.38, where the same
sequence persists everything). The 4.0 `atomic()` design — "don't commit an
existing transaction" — converts the old latent leak into permanent silent
data loss.
**`optimize()` (`db.py:2790`) and `rebuild_fts()` (`db.py:2752`) have the
identical leak** — both run `INSERT INTO fts(fts) VALUES(...)` via bare
`self.db.execute()`. The CLI escapes only because `cli.py:341` and
`cli.py:369` wrap the calls in `with db.conn:`.
Tellingly, the only `delete_where` example in the docs
(`docs/python-api.rst:983-990`) already wraps it in `with db.atomic():`,
papering over the bug, while every other single-op example needs no wrapper.
**Fix:** wrap all three in `with self.db.atomic():`; update the docs example.
These were the only leaky public write ops found — every other op
(insert/upsert/update/delete/lookup/transform/extract/create/add_column/
create_index/enable_fts/enable_counts/m2m/convert/duplicate) was verified to
leave `in_transaction=False`.
### 2. `drop-view` drops tables and `drop-table` drops views, silently
`cli.py:1728` (`drop-table`) and `cli.py:1800` (`drop-view`) both use
`db[name].drop()`. `Database.__getitem__` dispatches on the actual object
type, and `Table.drop()`/`View.drop()` each issue their matching `DROP`
statement.
Reproduced: `sqlite-utils drop-view t.db t` deleted **table `t`** with exit
code 0. The mirror case (`drop-table` on a view) also succeeds silently.
This is a data-loss footgun that contradicts the issue #657 table/view split
that is a headline 4.0 breaking change. Fixing it later converts a silent
"success" into an error — a semver problem, so it must land in 4.0.
**Fix:** one line each — use `db.table(name)` / `db.view(name)` and catch
`NoTable` / `NoView` for a clean `ClickException`.
### 3. Post-rc1 `insert({})` change lets `upsert` silently insert rows
The new `DEFAULT VALUES` branch (commit `b5d0080`,
`db.py:3184-3194`) checks `not list_mode and not all_columns` but never
checks the `upsert` flag:
- `db["t"].upsert({}, pk="id")` executes `INSERT INTO t DEFAULT VALUES` — a
brand-new row is written — and *then* raises an unrelated `KeyError: 'id'`
from the `last_pk` computation (`db.py:3742-3746`).
- `upsert_all([{}, {}], pk="id")` (2+ records skips the `last_pk` path)
**silently inserts two new rows on every call, no error at all**.
At rc1 both failed fast with `ZeroDivisionError` — ugly, but zero rows
written. Going from crash-without-mutation to silent insertion under
"upsert" semantics is a regression 4.0 would lock in.
Relatedly, the compound-pk auto-detection (commit `bfd74a3`,
`db.py:3557-3560`) makes records that *omit* the pk value reachable where
they previously raised `PrimaryKeyRequired` before touching the database:
`upsert_all([{"v": "a"}, {"v": "b"}])` on an existing pk table now inserts
with `id=NULL` (never conflicts) and appends new rows on every call
(verified: two calls → 4 rows). Same for compound pks with a missing
component. Not covered by any test.
**Fix:** raise a clean error in the empty-record and missing-pk-value paths
when `upsert=True` (an empty record has no pk value, so it can never be an
upsert).
### 4. `Migrations.apply()` transaction semantics are accidental — decide now
`sqlite_utils/migrations.py:84-95` runs each migration function and its
tracking-row insert with no transaction wrapper. Verified: a migration that
fails halfway leaves its partial side effects committed, records nothing,
and re-running re-executes the *entire* function including already-applied
statements — the classic double-apply hazard.
This matches sqlite-migrate, so either behavior is defensible — but 4.0
freezes the contract: adding per-migration transactions in 4.1 would break
migrations that manage their own transactions or run statements illegal
inside one (`VACUUM`, some `PRAGMA`s).
**Decide now:** wrap each migration in `db.atomic()`, or explicitly document
"migrations are not transactional; write idempotent steps" so the current
behavior is the contract rather than an accident.
Also decide now: **`_AppliedMigration.applied_at` is annotated
`datetime.datetime` (`migrations.py:21`) but is a `str` at runtime**
(`migrations.py:67` passes the TEXT column value straight through), and the
type is published via autodoc (`docs/migrations.rst:168-171`). Changing the
annotation to `str` now is a free one-liner (and the sqlite-migrate-
compatible choice); "fixing" it to a real datetime later breaks every
consumer doing string operations.
### 5. `enable_wal()` / `disable_wal()` commit open transactions
`ensure_autocommit_off()` (`db.py:456-472`) assigns `conn.isolation_level`,
and CPython's setter **commits any pending transaction** as a side effect.
Verified consequences:
- `with db.atomic(): insert(2); db.enable_wal(); insert(3); raise` → **all
rows persist despite the exception**, directly contradicting the
documented rollback guarantee (`docs/python-api.rst:255`), which is *the*
headline 4.0 semantic.
- A user's own `BEGIN` + insert + `enable_wal()` → their subsequent
`rollback()` is a no-op; the insert persists. This is exactly the
"unexpectedly committing an existing transaction" bug class `atomic()` was
introduced to eliminate (changelog, issue #755).
Pre-existing in 3.x, but 4.0 is the moment to fix.
**Fix:** make `enable_wal`/`disable_wal` raise (or skip the isolation dance)
when `conn.in_transaction`.
---
## Decisions to make now — cheap today, breaking after 4.0
### `Database.__enter__` / `__exit__` contract is unpinned
`__exit__` only calls `self.close()` (`db.py:408-417`), which silently rolls
back uncommitted changes. Verified: a raw `db.execute("insert ...")` inside
`with Database(p) as db:` is discarded on exit. This diverges from
`sqlite3.Connection`'s own context manager (commits on success, does *not*
close). The docs (`docs/python-api.rst:139-145`) say only "automatically
close the connection" — no mention of rollback — and **no test anywhere uses
`with Database(...)`**, so the contract is completely unpinned. Whichever way
this might later be "improved" is a breaking behavior change. Decide now,
add one docs sentence ("any uncommitted changes are rolled back") and a
pinning test. Note blockers 1–2 make this worse: after a `delete_where`,
exiting the `with` block discards everything.
### `atomic()` is broken on Python 3.12+ `autocommit` connections
`db.py:441-453` uses `conn.commit()`/`conn.rollback()`, documented no-ops
when `Connection.autocommit=True`. Verified on 3.13:
- `autocommit=True` connection → a successful `atomic()` block leaves the
transaction permanently open; rollback on exception is also a no-op.
- `autocommit=False` connection → the connection is always in a transaction,
so `atomic()` always takes the savepoint branch and never commits
anything.
`Database(filename_or_conn)` accepts arbitrary connections (`db.py:397`).
Either handle these modes (issue literal `COMMIT`/`ROLLBACK` statements, or
reject such connections) or document supported connection modes in
`python_api_atomic` now — changing commit timing later is breaking.
(Legacy `isolation_level=None` connections were verified to work correctly.)
### `atomic()` savepoint-branch semantics are intended but undocumented
If the connection is already in a transaction (user ran `BEGIN` or raw DML),
`atomic()` becomes a savepoint and its successful exit does **not** persist
anything until the user commits (verified: a user `rollback()` discards the
atomic block's writes). Reasonable semantics, but the `conn.in_transaction`
sniffing must be documented before it is locked — the docs currently
describe nesting only in terms of `atomic()`-inside-`atomic()`.
### `atomic()` issues plain deferred `BEGIN`
`db.py:442` — no way to request `BEGIN IMMEDIATE`. A future switch of the
default would change locking/`SQLITE_BUSY` behavior (breaking); adding an
`immediate=` parameter later is fine. Documenting "deferred" now costs one
sentence.
### No-op `-d/--detect-types` flag: keep or kill
`cli.py:942-947` still defines `-d/--detect-types` on `insert`/`upsert`
(help says "(default)"), deliberately tested as a no-op
(`tests/test_cli.py:2299,2322`). The `detect_types` parameter of
`insert_upsert_implementation` (`cli.py:1002`) is dead code — the body only
reads `no_detect_types`. If the flag is ever to be removed, 4.0 is the only
chance; keeping it as back-compat is defensible, but note
`--detect-types --no-detect-types` together silently favors the latter with
no conflict error.
### `--stop-before` is silently ignored for old `sqlite_migrate` objects
The duck-typing in `_compatible_migration_set` (`cli.py:3286-3289`) exists
precisely so migration files still doing `from sqlite_migrate import
Migrations` keep working — but `cli.py:3393` always passes `stop_before=` as
a **list**, and the old plugin's `apply()` does `if name == stop_before:`
against a string. Verified with the released 0.1b0: `--stop-before step2`
applied `step2` anyway. The CLI applies the exact migration the user asked
it not to, on the one upgrade path the duck typing exists to support.
Related: a **typo'd `--stop-before` name silently applies everything**,
including the migration you meant to stop before (unknown names simply never
match; no error from the CLI). Validation has to live in the CLI ("each
`--stop-before` value must match at least one set") because unqualified
names legitimately fan out across sets. Adding that error post-4.0 turns
currently-succeeding invocations into failures — decide now.
### Public-API validation via bare `assert`
User-facing errors raised via `assert` throughout `db.py` (370, 396, 1016,
1950, 2829, 3028, 3565, 3571, 3967, …). They vanish under `python -O`, and
`db.py:3028` carries `# TODO: Test this works (rolls back) - use better
exception:`. Converting `AssertionError` to `ValueError` later changes
exception types callers may catch — best done in a major, if ever.
---
## Should-fix polish (non-breaking later, but ugly for a stable release)
- **`insert`/`upsert` into a view name prints a raw traceback** —
`NoTable("Table v is actually a view")` from `cli.py:1154` is not
converted to `ClickException` (contrast `duplicate` at `cli.py:1676`).
- **Misleading `NoView` message** — `db.view("t")` where `t` is a table says
"View t does not exist" (`db.py:661`); the mirror case helpfully says
"Table v is actually a view" (`db.py:650`).
- **Stale `__getitem__` docstring** (`db.py:502`) says it "returns a Table
object"; it returns a `View` for views. Feeds autodoc.
- **Wrong `hash_id` doc in the `Table` docstring** (`db.py:1588`) — says
bool; it's a column-name string (`Optional[str]`).
- **`sqlite-utils migrate --list` writes to the database** —
`pending()`/`applied()` call `ensure_migrations_table`
(`migrations.py:48,65`), so `--list` creates the tracking table, performs
the one-way legacy sqlite-migrate schema upgrade, and — since `db_path`
has no `exists=True` (`cli.py:3338-3340`) — creates the database file
itself. A read-looking operation should not do any of that.
- **`applied()` has no `ORDER BY`** (`migrations.py:66-71`) — relies on
rowid order; `tests/test_cli_migrate.py:104-107` asserts that order.
Add `order_by="id"` to make `--list` deterministic by contract.
- **Duplicate migration names within a set**: both functions execute (side
effects committed) before an opaque `IntegrityError`; only the first is
recorded (`migrations.py:36-42`). A `ValueError` at registration would be
cheap and is effectively non-breaking to add now.
- **`pending()`/`applied()` return underscore-private dataclasses**
(`_Migration`/`_AppliedMigration`) that are excluded from autodoc
(`docs/migrations.rst:171`), so the `.name`/`.applied_at`/`.fn` fields
users must access are undocumented API.
- **`set:name` colon syntax is unvalidated** (`cli.py:3328` uses
`partition(":")`) — a set or migration name containing `:` can never be
targeted. Document/validate "no colons in names" now.
- **Bare `@migrations` decorator** gives an opaque `TypeError`
(`migrations.py:30`); a helpful message would be purely additive.
- **Tracer never sees transaction statements** — `atomic()` uses
`self.conn.execute()`/`conn.commit()` directly (`db.py:432-450`),
bypassing the tracer. Routing through `self.execute()` later would change
tracer output that downstream tests may assert on — cheap to decide now.
- **Order-dependent empty-dict semantics** (from `b5d0080`):
`insert_all([{}, {"v": "hi"}])` gives the empty record its column DEFAULTs,
but `insert_all([{"v": "hi"}, {}])` inserts explicit `NULL` for it — a
`NOT NULL ... DEFAULT` column succeeds in one ordering and raises
`IntegrityError` in the other.
- **Batch-size cliff when the first record is `{}`** (`db.py:3625-3629`) —
`num_columns` comes from the first record only, forcing `batch_size=1`
for the entire stream. Performance-only, degenerate input.
- **`pyproject.toml` build-system underpinned** — `requires = ["setuptools"]`
while the PEP 639 `license = "Apache-2.0"` string needs setuptools ≥
77.0.3; non-isolated sdist builds on older setuptools will fail. Pin
`setuptools>=77`.
- **Undocumented post-rc1 behavior changes** — neither upsert pk
auto-detection (`bfd74a3`) nor `insert({})`-uses-DEFAULT-VALUES
(`b5d0080`) is mentioned in `docs/` or the changelog. Note also that
`db.table(name).insert({})` only works for *existing* tables — on a
missing table it still raises `AssertionError: Tables must have at least
one column`, so the issue #759 title scenario is only partially covered.
---
## Verified sound — no action needed
- **sqlite-migrate on-disk compatibility** (the scariest area): tested
against the actual 0.1b0 sdist from PyPI. Same `_sqlite_migrations` table;
the legacy compound-pk `(migration_set, name)` schema is detected via
`table.pks != ["id"]` and upgraded in place (`migrations.py:97-117`); rows
are preserved and previously-applied migrations are **not** re-applied.
Both timestamp formats are identical. Tests cover both legacy layouts
(`tests/test_migrations.py:83-110`). No data-corruption risk for upgrading
users. The built-in `migrate` command is registered *after* plugin hooks
(`cli.py:3415-3417`), so a still-installed old plugin's command is
correctly overridden.
- Migration ordering is definition order (documented and tested); identity
is `(set name, migration name)` with a unique index; same name across sets
is fine. The new CLI is a strict superset of the old plugin's; file
discovery is now `sorted()` where the plugin iterated an unordered set.
- Nested savepoint rollback/release logic including exception paths
(`tests/test_atomic.py:44-120`); commit-time deferred-FK failure rolls
back cleanly; `transform()` inside an open transaction correctly uses
`PRAGMA defer_foreign_keys`; `_executescript` statement-splitting avoids
sqlite3's implicit commit. Savepoint naming via `secrets.token_hex(16)` is
collision-free.
- Compound-pk upsert detection happy path works for single and compound pks
on both the `ON CONFLICT` and `use_old_upsert` implementations; explicit
`pk=` still wins over detection; rowid/missing/hash_id tables still raise
`PrimaryKeyRequired` (`tests/test_upsert.py:52-97`).
- The table/view split (#657) is otherwise complete: `db.table()` on a view
raises `NoTable`, never returns a `View`; `db.view()` accepts no
table-only kwargs. The tracer-test fix (`401fb69`) and click pin
(`79117b9`, dev-group only) are correct.
- `__all__` exports are coherent (`Database`, `Migrations`,
`suggest_column_types`, `hookimpl`, `hookspec`); every `sqlite_utils.X`
docs reference resolves. The `pip` runtime dependency is genuinely
required (`cli.py:2967-2983` uses `run_module("pip")` for
`install`/`uninstall`). Classifiers match `requires-python`. No
deprecation debt: zero `DeprecationWarning` markers; clean under
`-W error::DeprecationWarning`.
---
## The transform() incoming-FK branch: hold for 4.1
The `update_incoming_fks` work on `claude/investigate-transform-fk-KMTZ7` is
right to hold:
- It is purely additive (`update_incoming_fks: bool = False` keyword +
`--update-incoming-fks` flag; default behavior unchanged), so 4.1 is safe
semver-wise.
- The branch predates the `atomic()` refactor — it still uses
`with self.db.conn:` directly in `transform()`, the exact pattern 4.0
eliminated (#755) — and has real issues: `_skip_fk_validation` is mutable
state on the whole `Database` object, and the incoming-FK rebuilds execute
`transform_sql()` output raw for referencing tables, silently dropping
their indexes (the 3.38 index-recreation logic only runs for the primary
table).
- One decision belongs to 4.0: whether `transform()` should eventually
refuse/warn *by default* when a rename would leave dangling incoming FKs.
Adding a warning later is fine; making it an error by default later is
breaking. If strict-by-default is the desired end state, add just the
guard in 4.0 and ship the auto-update machinery in 4.1.
---
## Suggested path to stable
1. Fix the five blockers — three `atomic()` wrappers (`delete_where`,
`optimize`, `rebuild_fts`), two CLI lookups (`drop-table`/`drop-view`),
the `upsert` guard in the DEFAULT VALUES / missing-pk paths, the
`applied_at: str` annotation, and the `in_transaction` guard in
`enable_wal`/`disable_wal`.
2. Make and document the decisions: `apply()` transactionality,
`Database.__exit__` semantics (+ pinning test), `atomic()` supported
connection modes and deferred-`BEGIN` note, `--detect-types` keep/kill,
`--stop-before` validation and old-plugin compat.
3. Cut **rc2** — the transaction-semantics fixes deserve one more candidate
round before the semantics freeze.