Skip to content

Add opt-in setting to automatically include fixtures in migrations on change - #269

Draft
benjie wants to merge 4 commits into
mainfrom
watch-fixtures
Draft

benjie wants to merge 4 commits into
mainfrom
watch-fixtures

Conversation

@benjie

@benjie benjie commented Oct 14, 2026

Copy link
Copy Markdown
Member

Untested; just to demonstrate approach

@benjie
benjie added this pull request to stack #267 October 14, 2026 14:13
@xvaara

xvaara commented Oct 14, 2026 •

Copy link
Copy Markdown

From claude opus (updated: #262 now builds on this PR, so my earlier comparison is superseded):

Short version: #262 is rebuilt on top of this PR. It now targets watch-fixtures and drops its own design. It adds the fixtures/auto/ folder from the Discord discussion, fixes a bug in the history lookup, and adds tests and docs. It's split into eight commits so you can review or cherry-pick them separately. The history fix is independent of the rest.

A bug here worth fixing on its own: in _reallyGetCommittedFixturesCache, the regex captures (^|\n) before the path, so const [, relativePath] = match reads that group. Every committed fixture is keyed by "" or "\n", none ever matches, and every fixture is included again in every migration. #262's first commit fixes it, and normalises ./x.sql to x.sql.

What #262 adds on top of this PR:

  • Only fixtures/auto/ is included automatically, and creating the folder is the opt-in. The autoIncludeFixtures setting is removed. Without the folder, the committed history isn't read at all. Other fixtures, such as indexes, are only included manually.
  • --!include inside fixtures/auto/ is an error. Nothing runs twice through nesting, and no DAG is needed.
  • --! no-transaction inside fixtures/auto/ is an error too, as you suggested, instead of a warning that's easy to miss during commit. The automatic section always runs inside the migration's transaction, so such fixtures belong in another folder, included manually.
  • The automatic fixtures go after the headers and --! no-transaction, and only that section is stripped on uncommit. With this PR alone:
    • commit loses --! Message: (the file is 000001.sql, not 000001-add-example.sql);
    • a current.sql that starts with --! no-transaction runs in a transaction;
    • the strip on uncommit removes everything before --!endAuto, including headers such as a -m message.
  • Nothing can --!include an auto fixture, neither the current migration nor a fixture it includes, as you asked in review. Auto fixtures only ever run in the automatic section, so there's nothing already included to detect, and the TODO about include spellings goes away.
  • New fixtures that are empty or only comments are skipped.
  • Tests and docs: 19 tests (14 without a database, 5 lifecycle) and a README section. Before the setting was removed, I ran the 17 tests that existed then against this PR with it switched on, and 14 of them failed.

Corrections to my earlier comments:

Open questions:

  • Projects that already have a fixtures/auto/ folder for something else would switch this on when they upgrade. Is a release note enough, or would a more distinctive folder name be better? At least any --!include of files in such a folder now fails with a clear error.
  • One limitation remains: pg-minify 1.6.3 doesn't understand dollar quoting. Edits after -- inside a $tag$…$tag$ string go undetected, and some valid SQL (for example comment on function f() is $$Don't…$$;) makes it throw.

@benjie
benjie force-pushed the watch-fixtures branch 2 times, most recently from 89720f6 to 03d89cf Compare October 14, 2026 14:59
Base automatically changed from postprocess-once to main October 14, 2026 15:02
@benjie
benjie removed this pull request from stack #267 October 14, 2026 15:02
@benjie
benjie force-pushed the watch-fixtures branch 2 times, most recently from ee03d5f to 404164c Compare October 15, 2026 08:45
xvaara added a commit to xvaara/graphile-migrate that referenced this pull request Oct 16, 2026
uncommit again checks the compiled current migration, as in graphile#269, so it
refuses while a fixture has pending changes; commit or revert the edit
first. Nothing is lost either way, and readCurrentMigration keeps its
original signature.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants