sneak/pixa
1
0
Fork 0
Code Issues 3 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

feat: parse version prefix from migration filenames #33

Merged
sneak merged 2 commits from feat/parse-migration-version into main 2026-03-18 03:18:39 +01:00
Conversation 3 Commits 2 Files Changed 6 +253 -103
clawbot commented 2026-03-15 19:24:50 +01:00
Collaborator
Copy Link

Closes #28

Migration filenames now follow the pattern <version>_<description>.sql (e.g. 001_initial_schema.sql). The version stored in schema_migrations is the numeric prefix only, not the full filename stem.

Changes

  • ParseMigrationVersion() — new exported function that extracts the numeric prefix from migration filenames. Validates that the prefix is purely numeric and rejects malformed filenames (empty prefix, non-numeric characters, leading underscore).
  • Renamed 001.sql → 001_initial_schema.sql — migration files can now have descriptive names while the tracked version remains 001. This is safe pre-1.0.0 (no installed base).
  • Deduplicated migration logic — runMigrations() and ApplyMigrations() now share a single applyMigrations() implementation, plus extracted collectMigrations() and ensureMigrationsTable() helpers.
  • Unit tests — TestParseMigrationVersion covers valid patterns (version-only, with description, multi-digit, multiple underscores) and error cases (empty, leading underscore, non-numeric, mixed alphanumeric). TestApplyMigrations and TestApplyMigrationsIdempotent verify end-to-end migration application against an in-memory SQLite database.
Closes #28 Migration filenames now follow the pattern `<version>_<description>.sql` (e.g. `001_initial_schema.sql`). The version stored in `schema_migrations` is the numeric prefix only, not the full filename stem. ## Changes - **`ParseMigrationVersion()`** — new exported function that extracts the numeric prefix from migration filenames. Validates that the prefix is purely numeric and rejects malformed filenames (empty prefix, non-numeric characters, leading underscore). - **Renamed `001.sql` → `001_initial_schema.sql`** — migration files can now have descriptive names while the tracked version remains `001`. This is safe pre-1.0.0 (no installed base). - **Deduplicated migration logic** — `runMigrations()` and `ApplyMigrations()` now share a single `applyMigrations()` implementation, plus extracted `collectMigrations()` and `ensureMigrationsTable()` helpers. - **Unit tests** — `TestParseMigrationVersion` covers valid patterns (version-only, with description, multi-digit, multiple underscores) and error cases (empty, leading underscore, non-numeric, mixed alphanumeric). `TestApplyMigrations` and `TestApplyMigrationsIdempotent` verify end-to-end migration application against an in-memory SQLite database.
clawbot added 1 commit 2026-03-15 19:24:50 +01:00
feat: parse version prefix from migration filenames
All checks were successful
check / check (push) Successful in 1m51s
Details
63f8cd1bd0 
Migration filenames now follow the pattern <version>_<description>.sql
(e.g. 001_initial_schema.sql). The version stored in schema_migrations
is the numeric prefix only, not the full filename stem.

Changes:
- Add ParseMigrationVersion() to extract numeric prefix from filenames
- Validate version is purely numeric, reject malformed filenames
- Rename 001.sql to 001_initial_schema.sql for clarity
- Deduplicate runMigrations/ApplyMigrations via shared applyMigrations()
- Add comprehensive unit tests for version parsing and migration apply

Closes #28
sneak commented 2026-03-15 19:39:53 +01:00
Owner
Copy Link

why are there two applyMigrations

why are there two applyMigrations
clawbot added the needs-rework label 2026-03-15 19:40:40 +01:00
clawbot self-assigned this 2026-03-15 22:09:28 +01:00
clawbot added 1 commit 2026-03-17 09:53:24 +01:00
refactor: consolidate applyMigrations into single exported function
All checks were successful
check / check (push) Successful in 1m8s
Details
6a248756b5 
Remove the unexported applyMigrations() and the runMigrations() method.
ApplyMigrations() is now the single implementation, accepting context
and an optional logger. connect() calls it directly.

All callers updated to pass context.Background() and nil logger.
clawbot commented 2026-03-17 09:53:33 +01:00
Author
Collaborator
Copy Link

Rework: consolidated duplicate applyMigrations

The unexported applyMigrations() function and the runMigrations() method have been removed. There is now a single ApplyMigrations(ctx context.Context, db *sql.DB, log *slog.Logger) that serves both the production path (called from connect() with context and logger) and the test path (called with context.Background() and nil logger).

Changes:

  • Removed unexported applyMigrations(ctx, db, log) standalone function
  • Removed runMigrations(ctx) method on Database
  • Expanded exported ApplyMigrations signature to accept context.Context and optional *slog.Logger
  • connect() now calls ApplyMigrations(ctx, s.db, s.log) directly
  • Updated all test callers across database, imgcache, and handlers packages

All tests pass, docker build . succeeds.

## Rework: consolidated duplicate applyMigrations The unexported `applyMigrations()` function and the `runMigrations()` method have been removed. There is now a single `ApplyMigrations(ctx context.Context, db *sql.DB, log *slog.Logger)` that serves both the production path (called from `connect()` with context and logger) and the test path (called with `context.Background()` and `nil` logger). **Changes:** - Removed unexported `applyMigrations(ctx, db, log)` standalone function - Removed `runMigrations(ctx)` method on `Database` - Expanded exported `ApplyMigrations` signature to accept `context.Context` and optional `*slog.Logger` - `connect()` now calls `ApplyMigrations(ctx, s.db, s.log)` directly - Updated all test callers across `database`, `imgcache`, and `handlers` packages All tests pass, `docker build .` succeeds.
clawbot added needs-review and removed needs-rework labels 2026-03-17 09:54:16 +01:00
clawbot commented 2026-03-17 10:08:45 +01:00
Author
Collaborator
Copy Link

Review: PR #33 — feat: parse version prefix from migration filenames

Policy Compliance

No policy violations found. Specifically:

  • No modifications to .golangci.yml, Makefile, Dockerfile, or CI config
  • No weakened assertions or test shortcuts
  • New exported function has unit tests
  • go fmt passes, linter passes (0 issues)
  • External references unchanged; no new unpinned dependencies
  • Migration files remain in internal/database/schema/ (pre-existing path; not changed by this PR)

Requirements Checklist (Issue #28)

  • Parse version out of migration filename — ParseMigrationVersion() extracts the numeric prefix from <version>_<description>.sql filenames. Validates purely numeric prefix, rejects empty prefix, leading underscore, non-numeric characters.
  • Migration files use descriptive names — 001.sql renamed to 001_initial_schema.sql; tracked version remains 001.
  • Consolidate duplicate applyMigrations (sneak's comment) — single exported ApplyMigrations(ctx, db, log) replaces both the unexported standalone function and the runMigrations method. All callers updated.
  • Unit tests — TestParseMigrationVersion (9 cases: valid patterns + error cases), TestApplyMigrations (end-to-end against in-memory SQLite), TestApplyMigrationsIdempotent (double-apply correctness).

Code Quality

  • Clean decomposition: collectMigrations(), ensureMigrationsTable(), ApplyMigrations() are well-separated concerns
  • Optional *slog.Logger parameter with nil-safe checks — clean API for both production and test paths
  • Error messages include filename context for debuggability
  • Edge cases handled: empty filename, leading underscore, mixed alphanumeric, no-underscore filenames
  • All existing test callers (handlers_test.go, stats_test.go, testutil_test.go) updated to new signature

Build Result

docker build . PASSES — fmt-check ✅, lint (0 issues) ✅, all tests pass ✅, binary builds ✅

Rebased onto main — already up to date, build confirmed.

Verdict: PASS

## Review: [PR #33](https://git.eeqj.de/sneak/pixa/pulls/33) — feat: parse version prefix from migration filenames ### Policy Compliance No policy violations found. Specifically: - No modifications to `.golangci.yml`, Makefile, Dockerfile, or CI config - No weakened assertions or test shortcuts - New exported function has unit tests - `go fmt` passes, linter passes (0 issues) - External references unchanged; no new unpinned dependencies - Migration files remain in `internal/database/schema/` (pre-existing path; not changed by this PR) ### Requirements Checklist ([Issue #28](https://git.eeqj.de/sneak/pixa/issues/28)) - [x] **Parse version out of migration filename** — `ParseMigrationVersion()` extracts the numeric prefix from `<version>_<description>.sql` filenames. Validates purely numeric prefix, rejects empty prefix, leading underscore, non-numeric characters. - [x] **Migration files use descriptive names** — `001.sql` renamed to `001_initial_schema.sql`; tracked version remains `001`. - [x] **Consolidate duplicate applyMigrations** (sneak's comment) — single exported `ApplyMigrations(ctx, db, log)` replaces both the unexported standalone function and the `runMigrations` method. All callers updated. - [x] **Unit tests** — `TestParseMigrationVersion` (9 cases: valid patterns + error cases), `TestApplyMigrations` (end-to-end against in-memory SQLite), `TestApplyMigrationsIdempotent` (double-apply correctness). ### Code Quality - Clean decomposition: `collectMigrations()`, `ensureMigrationsTable()`, `ApplyMigrations()` are well-separated concerns - Optional `*slog.Logger` parameter with nil-safe checks — clean API for both production and test paths - Error messages include filename context for debuggability - Edge cases handled: empty filename, leading underscore, mixed alphanumeric, no-underscore filenames - All existing test callers (`handlers_test.go`, `stats_test.go`, `testutil_test.go`) updated to new signature ### Build Result `docker build .` **PASSES** — fmt-check ✅, lint (0 issues) ✅, all tests pass ✅, binary builds ✅ Rebased onto `main` — already up to date, build confirmed. ### Verdict: **PASS**
clawbot added merge-ready and removed needs-review labels 2026-03-17 10:09:35 +01:00
clawbot removed their assignment 2026-03-17 10:09:35 +01:00
sneak was assigned by clawbot 2026-03-17 10:09:35 +01:00
sneak merged commit 9c29cb57df into main 2026-03-18 03:18:39 +01:00
sneak deleted branch feat/parse-migration-version 2026-03-18 03:18:39 +01:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#33
Delete Branch "feat/parse-migration-version"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?