feat(hook): migrations/ preferred location + --hook/SQLPUSH_HOOK override (defaults + override, the alembic pattern) - #9
Merged
Conversation
…rrors) Discovery now checks migrations/sqlpush.py first (preferred: lives next to the chain, no root clutter) then the repo-root sqlpush.py (backwards compat) — first match wins; root-only setups behave exactly as in 0.6.0 (the existing root-location tests now double as the fallback pin, unchanged). Two correctness requirements beyond the owner's snippet: - typed errors name the file that ACTUALLY loaded, in the candidate spelling (load-time errors use the matched candidate directly; accessors map the loaded module's __file__ back to its candidate — posix-formatted so messages and tests read migrations/sqlpush.py). - sys.path still appends the CWD regardless of which candidate loaded, never the hook's own directory (migrations/ on sys.path would be wrong for consumer package imports) — pinned by test. Filed under Added rather than Changed: the only behavior change vs 0.6.0 is the exotic both-files-present case (migrations/ wins); root hook users see nothing move.
Project hook section now states the two candidate locations and the first-match order (migrations/sqlpush.py preferred — next to the chain; repo-root sqlpush.py as the backwards-compat fallback), moves the example header to the preferred location, and updates the error contract sentence (the message names the file that actually loaded) and the shadowing note (append-the-CWD applies whatever location loaded; the migrations candidate has no shadowing concern but the same load-by-path mechanics).
Explicit override on top of candidate discovery (the alembic -c equivalent): --hook PATH on every verb, or $SQLPUSH_HOOK, loads the hook from any location. Precedence: flag > env > candidates; an explicit path that does not exist fails loud as a typed HookError naming that exact path — never a silent fallback. Errors from an overridden hook name the loaded file in the given spelling (load-time label registry), and sys.path still appends the CWD, never the loaded file's directory.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hook location flexibility, following the pattern alembic set with its ini file and its
-cflag: sensible defaults, plus an explicit way to point anywhere. This came out of dogfooding the 0.6.0 hook in a consumer repo.Discovery now checks
migrations/sqlpush.pyfirst (the file lives next to the chain, the way alembic'senv.pylives inside its script location), then the repo root as the backwards-compatible fallback. First match wins, and error messages name the file that actually loaded:migrations/sqlpush.py: missing get_dsn(). The CWD is what gets appended tosys.path, whichever location the hook came from. If you only have a rootsqlpush.py, nothing changes for you.Anyone who wants the hook somewhere else can say so:
--hook PATHon every verb, or theSQLPUSH_HOOKenvironment variable. Precedence is flag, then env, then discovery. An explicit path that does not exist fails loud with a typed error naming that path; it never falls back to the candidates (there is a test with a loadable candidate planted as temptation). An emptySQLPUSH_HOOKcounts as unset, which is what CI systems that export empty variables need.Commits:
41ad2aaand62dc7dcare the preferred location;544b2fcanda65006eare the override;8ec2814adds the review pins (empty-env behavior, flag-beats-candidates tested directly instead of by transitivity) and a wording fix.Verification: 194 passed + 1 xfailed (181 baseline plus 13 new), ruff/format/ty clean, and every behavior test was observed red before its implementation. The review approved the branch after checking the precedence in the code itself, probing the label registry against symlinks and path oddities, and confirming that the no-hook and root-hook paths are byte-identical to 0.6.0.
The CHANGELOG carries the two Added bullets and the README documents both locations and the precedence. On merge this is a minor by the repo's convention (a new flag and a new discovery location are new functionality):
v0.7.0, though0.6.1is defensible since it extends the hook published yesterday. Owner's call at cut time.