Add internal test exports and improve golangci-lint configuration#31
Add internal test exports and improve golangci-lint configuration#31
Conversation
- Introduced `exports_internal_test.go` for internal testing of private methods. - Updated `.golangci.yaml` with additional linter settings and exclusions. - Fixed typos and improved documentation consistency across files. - Namespaced test imports properly for clarity.
|
Warning Rate limit exceeded@AlphaOne1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughContext.Context was threaded through public APIs and internal flows; DB/tx operations were made context-aware (BeginTx/ExecContext/QueryContext); internal helpers were exposed as test-only wrappers; many tests moved to external package to exercise the public API; CI comment punctuation and linter config were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Morpher
participant Dialect
participant DB
participant Migration
Caller->>Morpher: Run(ctx, db, ...)
Morpher->>Dialect: EnsureMigrationTableExists(ctx, db, table)
Dialect->>DB: BeginTx(ctx, ...)
DB-->>Dialect: tx
Dialect->>DB: ExecContext(ctx, create-table SQL)
alt Exec error
DB-->>Dialect: error
Dialect->>DB: Rollback(tx)
Dialect-->>Morpher: error
else Exec success
DB-->>Dialect: ok
Dialect->>DB: Commit()
Dialect-->>Morpher: nil
end
Morpher->>Morpher: AppliedMigrations(ctx, db, table)
Morpher->>Morpher: applyMigrations(ctx, db, lastMigration)
loop for each migration
Morpher->>DB: BeginTx(ctx, ...)
Morpher->>Migration: Migrate(ctx, tx)
Migration->>DB: ExecContext(ctx, ...)
alt migration error or ctx canceled
DB-->>Morpher: error
Morpher->>DB: Rollback(tx)
else success
Morpher->>Dialect: RegisterMigration(ctx, tx, id, table)
Morpher->>DB: Commit()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
.github/workflows/codeql.yml (1)
85-85: Nit: Prefer “analyze step” (two words) or quote the exact step/action nameHyphenating as “analyze-step” reads awkwardly and is inconsistent with the rest of the file. Consider either plain “analyze step” or explicitly naming the action/step to avoid ambiguity.
- # If the analyze-step fails for one of the languages you are analyzing with + # If the analyze step fails for one of the languages you are analyzing withdialects.go (1)
33-37: Optional: avoid redundant Rollback callsPattern uses a deferred Rollback plus explicit Rollback on errors, causing Rollback to be invoked twice on error paths (deferred one returns ErrTxDone, ignored). Safe but noisy. If you want to tighten this, prefer a defer that only rolls back when needed (named error + check) or drop the explicit defer and rely on explicit rollbacks and commit.
Also applies to: 39-43
file_migration.go (1)
114-121: Optional: increase Scanner buffer to handle long SQL linesbufio.Scanner has a default token limit (~64K). Long statements or very long lines can trip “token too long”. Consider configuring a larger buffer or switching to bufio.Reader with custom splitting.
Example:
scanner := bufio.NewScanner(r) +// Allow longer lines/statements (up to 10MB here). +scanner.Buffer(make([]byte, 0, 64*1024), 10*1024*1024).golangci.yaml (1)
93-95: Issues block duplicated after moveOnce you move exclude-rules into issues (previous comment), keep a single issues section. Combine max-issues-per-linter and max-same-issues there.
dialects_test.go (1)
92-99: Rename short variable for clarity and lint complianceStatic analysis flagged variable name ‘d’ as too short for scope. Rename to dialect for readability.
- d := dmorph.BaseDialect{ + dialect := dmorph.BaseDialect{ @@ - assert.Error(t, d.EnsureMigrationTableExists(db, "test"), "expected error") + assert.Error(t, dialect.EnsureMigrationTableExists(db, "test"), "expected error")migration.go (2)
283-289: Optional (Go 1.22+): use integer range for indicesYou can simplify the index loop if targeting Go 1.22+.
- for i := 0; i < len(appliedMigrations); i++ { - if appliedMigrations[i] != m.Migrations[i].Key() { + for i := range appliedMigrations { + if appliedMigrations[i] != m.Migrations[i].Key() { return ErrMigrationsUnrelated } }Confirm your minimum Go version before applying.
214-236: Optional: tighten transaction rollback defer within loopDeferring tx.Rollback() inside the loop piles up defers. It’s safe but noisy. Alternative is a defer that only rolls back on error using a named error, or perform explicit rollback/commit without a defer and ensure all error paths join rollback errors as you already do.
migration_test.go (3)
172-179: Use require for preconditions to fail fastWhere a failure makes subsequent assertions meaningless (e.g., directory opens, morpher creation, pragma exec), prefer require.* to abort the test early.
- assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") + require.NoError(t, migrationsDirErr, "migrations directory could not be opened")…and similarly for morpherErr, execErr, etc.
Also applies to: 538-543, 567-576, 601-605
470-476: Deduplicate test string constant“utter nonsense” appears multiple times; extract to a const to appease goconst and improve intent.
+const garbageSQL = "utter nonsense" @@ - dialect.CreateTemplate = "utter nonsense" + dialect.CreateTemplate = garbageSQL @@ - dialect.AppliedTemplate = "utter nonsense" + dialect.AppliedTemplate = garbageSQL @@ - dmorph.WithMigrationFromFile("testData/01_base_table.sql")) + dmorph.WithMigrationFromFile("testData/01_base_table.sql"))(Adapt other occurrences accordingly.)
Also applies to: 502-508, 534-537
569-575: Type assertion: check ok for safetyAlthough DialectSQLite returns BaseDialect and the assertion should succeed, it’s better style to check ok and fail fast to avoid accidental changes breaking the tests.
- d, _ := morpher.Dialect.(dmorph.BaseDialect) + d, ok := morpher.Dialect.(dmorph.BaseDialect) + require.True(t, ok, "expected Dialect to be BaseDialect")Also applies to: 625-627
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/codeql.yml(1 hunks).golangci.yaml(2 hunks)dialects.go(2 hunks)dialects_test.go(5 hunks)exports_internal_test.go(1 hunks)file_migration.go(4 hunks)file_migration_test.go(6 hunks)migration.go(9 hunks)migration_test.go(23 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
dialects_test.go (8)
dialects.go (1)
BaseDialect(15-19)dialect_csvq.go (1)
DialectCSVQ(7-22)dialect_db2.go (1)
DialectDB2(7-31)dialect_mssql.go (1)
DialectMSSQL(7-27)dialect_mysql.go (1)
DialectMySQL(7-16)dialect_oracle.go (1)
DialectOracle(7-31)dialect_postgres.go (1)
DialectPostgres(7-22)dialect_sqlite.go (1)
DialectSQLite(7-22)
exports_internal_test.go (1)
migration.go (1)
Morpher(73-78)
migration_test.go (5)
migration.go (16)
Run(294-302)WithDialect(84-90)WithMigrations(93-99)ErrMigrationsTooOld(45-45)ErrMigrationsUnrelated(24-24)ErrMigrationsUnsorted(28-28)Migration(55-58)Morpher(73-78)Dialect(48-52)ErrNoDialect(31-31)ErrNoMigrations(34-34)ErrNoMigrationTable(37-37)ErrMigrationTableNameInvalid(40-40)NewMorpher(132-149)WithLog(103-109)WithTableName(113-127)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(68-85)WithMigrationFromFileFS(58-64)FileMigration(18-22)WithMigrationFromFile(35-54)exports_internal_test.go (1)
TmigrationOrder(10-10)dialects.go (1)
BaseDialect(15-19)
🪛 golangci-lint (2.2.2)
dialects_test.go
122-122: variable name 'd' is too short for the scope of its usage
(varnamelen)
exports_internal_test.go
8-8: TapplyStepsStream is a global variable
(gochecknoglobals)
9-9: TmigrationFromFileFS is a global variable
(gochecknoglobals)
10-10: TmigrationOrder is a global variable
(gochecknoglobals)
migration.go
283-283: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
migration_test.go
62-62: right hand must be only type assertion
(forcetypeassert)
97-97: right hand must be only type assertion
(forcetypeassert)
168-168: right hand must be only type assertion
(forcetypeassert)
172-172: require-error: for error assertions use require
(testifylint)
203-203: right hand must be only type assertion
(forcetypeassert)
283-283: right hand must be only type assertion
(forcetypeassert)
471-471: string utter nonsense has 3 occurrences, make it a constant
(goconst)
538-538: require-error: for error assertions use require
(testifylint)
567-567: require-error: for error assertions use require
(testifylint)
601-601: require-error: for error assertions use require
(testifylint)
604-604: require-error: for error assertions use require
(testifylint)
🔇 Additional comments (9)
dialects.go (1)
12-15: Comment grammar fix looks goodThe pluralization change (“operations”) improves clarity. No functional impact.
file_migration.go (3)
74-77: LGTM: readability-only blank lineThe blank line after logging improves readability. No functional change.
87-104: Doc comment addition is fineExported-style comment for migrationFromFileFS improves clarity. No functional change.
142-149: LGTM: clarified comment about final-statement cleanupComment improvement matches behavior. No logic change.
file_migration_test.go (2)
4-17: External test package switch is correctImporting dmorph and qualifying usages aligns tests with public API. Good move.
100-121: Good negative tests for applyStepsStreamFeeding malformed SQL to assert errors is appropriate. No changes requested.
dialects_test.go (1)
4-15: External test package conversion is appropriateQualified usage of dmorph types/functions improves API-focused testing.
migration.go (1)
232-241: LGTM: clearer rollback comments and spacingComment rewording and blank lines around error joins improve readability. Logic unchanged.
Also applies to: 243-253
migration_test.go (1)
62-65: Type assertion on embed.FS is safe—no change neededThe embedded
testMigrationsDiris of type embed.FS, which implements fs.ReadDirFS and fs.SubFS. Calls to fs.Sub(testMigrationsDir, …) return an embed.FS that always implements fs.ReadDirFS, so the existing assertions won’t panic. Since the API for WithMigrationsFromFS requires fs.ReadDirFS, the current code is correct and consistent with the signature.No edits are required in migration_test.go.
| default: all | ||
| #enable: | ||
| # # Default linters | ||
| # - errcheck | ||
| # - govet | ||
| # - ineffassign | ||
| # - staticcheck | ||
| # - unused | ||
| # # Additional linters | ||
| # - gosec | ||
| # - misspell | ||
| # - revive | ||
| # - bodyclose | ||
| # - noctx | ||
|
|
||
| disable: | ||
| - exhaustruct | ||
| - forbidigo | ||
| - noinlineerr | ||
| - nonamedreturns | ||
| - wsl | ||
| - wsl_v5 | ||
|
|
There was a problem hiding this comment.
Invalid key: use linters.enable-all instead of linters.default
golangci-lint doesn’t support linters.default: all. To enable all and selectively disable, use enable-all: true.
Apply this diff:
linters:
- default: all
+ enable-all: true
disable:
- exhaustruct
- forbidigo
- noinlineerr
- nonamedreturns
- wsl
- wsl_v5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default: all | |
| #enable: | |
| # # Default linters | |
| # - errcheck | |
| # - govet | |
| # - ineffassign | |
| # - staticcheck | |
| # - unused | |
| # # Additional linters | |
| # - gosec | |
| # - misspell | |
| # - revive | |
| # - bodyclose | |
| # - noctx | |
| disable: | |
| - exhaustruct | |
| - forbidigo | |
| - noinlineerr | |
| - nonamedreturns | |
| - wsl | |
| - wsl_v5 | |
| linters: | |
| enable-all: true | |
| disable: | |
| - exhaustruct | |
| - forbidigo | |
| - noinlineerr | |
| - nonamedreturns | |
| - wsl | |
| - wsl_v5 |
🤖 Prompt for AI Agents
In the .golangci.yaml file between lines 14 and 23, replace the key 'default:
all' with 'enable-all: true' under the linters section to correctly enable all
linters. Keep the 'disable' list as is to selectively disable specific linters.
This change aligns with golangci-lint's configuration syntax requirements.
| exclusions: | ||
| warn-unused: true | ||
|
|
||
| rules: | ||
| - path: main_test.go | ||
| - path: _test\.go | ||
| linters: | ||
| - gosec | ||
| - cyclop | ||
| - dupl | ||
| - dupword | ||
| - err113 | ||
| - funlen | ||
| - gocognit | ||
| - maintidx | ||
| - nestif | ||
|
|
||
| - path: examples/ | ||
| linters: | ||
| - err113 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Misplaced and non-standard: move “exclusions” under issues.exclude-rules
The “exclusions” block and “warn-unused” aren’t valid under linters. Path- and linter-specific rules belong under issues.exclude-rules. “warn-unused” is not a recognized top-level option.
Proposed structure:
- exclusions:
- warn-unused: true
-
- rules:
- - path: _test\.go
- linters:
- - cyclop
- - dupl
- - dupword
- - err113
- - funlen
- - gocognit
- - maintidx
- - nestif
-
- - path: examples/
- linters:
- - err113
+issues:
+ max-issues-per-linter: 0
+ max-same-issues: 0
+ exclude-rules:
+ # Example: ignore gochecknoglobals in internal test-exports file
+ - path: ^exports_internal_test\.go$
+ linters:
+ - gochecknoglobals
+ # Example: restrict heavy complexity checks to tests only by excluding them elsewhere
+ - path: ^(?!.*_test\.go).*$ # non-test files
+ linters:
+ - cyclop
+ - dupl
+ - dupword
+ - funlen
+ - gocognit
+ - maintidx
+ - nestif
+ # Only run err113 in examples by excluding it elsewhere, or vice versa per intent
+ - path: ^examples/
+ linters:
+ - $allNote: golangci-lint doesn’t support “only run these linters for this path”. The approach is to exclude linters in paths where they shouldn’t run.
Please confirm against the official schema; if desired, I can tailor a precise exclude-rules matrix for your intent.
🌐 Web query:
Is "exclusions" a valid top-level option in GolangCI-Lint configuration?
Does GolangCI-Lint support a top-level "warn-unused" setting?
Should path- and linter-specific rules be placed under an "exclusions" block, or under "issues.exclude-rules" instead?
💡 Result:
"exclusions" is not a valid top-level option in GolangCI-Lint's configuration.
GolangCI-Lint does not have a top-level "warn-unused" setting.
Path- and linter-specific rules should be placed under "issues.exclude-rules", not an "exclusions" block.
Details and Evidence
-
No "exclusions" top-level block:
The official GolangCI-Lint [documentation][7], open-source config references[4][9], and blog articles show the recognized top-level keys such asrun,output,issues,linters, andlinters-settings, but none refer to or support anexclusionskey. Further, usage of exemptions is always specified within theissuesblock, underexcludeorexclude-rules[4][9]. -
No "warn-unused" top-level setting:
There is no mention of awarn-unusedoption in the config file or CLI switches[7]. Unused code warnings are handled via theunusedlinter and its specific settings, which are configured withinlinters-settings: unused: ...[8][10]. -
Where to place rules for excluding issues:
- To exclude issues by path, linter, or code pattern, use the
issues.exclude-ruleslist within your configuration:[3][4]issues: exclude-rules: - path: _test\.go linters: - errcheck text: "error return value not checked"
This is supported directly in [documentation][3] and observed in community examples. - There is only one place for structured exclusion rules;
issues.excludeis also supported, but for regex patterns only, not rule objects.
- To exclude issues by path, linter, or code pattern, use the
Additional context
- The
run.skip-dirsandrun.skip-filesoptions can be used at top-level (underrun) for broad file/directory exclusions. - The confusion may arise from similar-sounding options in other linter tools or extensions.
Conclusion
For all issue/path/linter-specific exclusions, use issues.exclude-rules. "exclusions" and "warn-unused" are not valid top-level settings in GolangCI-Lint.[3][4][7][8][9]
Citations:
- 1: https://www.jetbrains.com/help/go/configuring-golangci-lint-in-the-go-linter-plugin.html
- 2: nolintlint does not report unused issue when disabled linter is disabled in configuration golangci/golangci-lint#2920
- 3: Relative paths handling is inconsistent between configuration options, hence unable to run golangci-lint from a subdirectory golangci/golangci-lint#3717
- 4: https://olegk.dev/go-linters-configuration-the-right-version
- 5: https://golangci-lint.com/how-to-ignore-lint-errors-in-golang/
- 6: https://megalinter.io/7.1.0/descriptors/go_golangci_lint/
- 7: https://golangci-lint.run/usage/configuration/
- 8: unused doesn't work with "exported-is-used" and "exported-fields-are-used" linter settings golangci/golangci-lint#4218
- 9: https://gist.github.com/brpaz/dc3635f52ccfed2eb5772c4cb484cab6
- 10: https://golangci-lint.run/usage/linters/
Move “exclusions” and “warn-unused” into issues.exclude-rules
GolangCI-Lint does not recognize a top-level exclusions block or warn-unused setting. All path- and linter-specific ignores must live under issues.exclude-rules.
Please update .golangci.yaml (lines 24–42) as follows:
- exclusions:
- warn-unused: true
-
- rules:
- - path: _test\.go
- linters:
- - cyclop
- - dupl
- - dupword
- - err113
- - funlen
- - gocognit
- - maintidx
- - nestif
-
- - path: examples/
- linters:
- - err113
+issues:
+ # Prevent any unused warnings; configure via the “unused” linter instead
+ max-issues-per-linter: 0
+ max-same-issues: 0
+ exclude-rules:
+ # Don’t run complexity checks on non-test files
+ - path: ^(?!.*_test\.go).*$ # all files except *_test.go
+ linters:
+ - cyclop
+ - dupl
+ - dupword
+ - funlen
+ - gocognit
+ - maintidx
+ - nestif
+
+ # Only ignore err113 in examples/
+ - path: ^examples/
+ linters:
+ - err113Tags:
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .golangci.yaml around lines 24 to 42, the "exclusions" and "warn-unused"
settings are incorrectly placed at the top level. Move these settings under the
"issues.exclude-rules" section to comply with GolangCI-Lint configuration
requirements. Specifically, nest the existing path and linter-specific ignore
rules inside "issues.exclude-rules" and set "warn-unused" as a property within
that section.
| settings: | ||
| cyclop: | ||
| max-complexity: 25 | ||
|
|
||
| depguard: | ||
| rules: | ||
| main: | ||
| files: | ||
| - $all | ||
| - "!$test" | ||
| - "!**/examples/**/*" | ||
| allow: | ||
| - $gostd | ||
| - gopkg.in/yaml.v3 | ||
| - github.com/Masterminds/sprig/v3 | ||
| test: | ||
| files: | ||
| - $test | ||
| - "**/examples/**/*" | ||
| allow: | ||
| - $gostd | ||
| - gopkg.in/yaml.v3 | ||
| - github.com/Masterminds/sprig/v3 | ||
| - github.com/stretchr/testify/assert | ||
| - github.com/AlphaOne1/dmorph | ||
| - modernc.org/sqlite | ||
|
|
||
| exhaustive: | ||
| default-signifies-exhaustive: true | ||
|
|
||
| mnd: | ||
| ignored-numbers: | ||
| - "2" | ||
|
|
||
| paralleltest: | ||
| ignore-missing: true | ||
|
|
||
| perfsprint: | ||
| errorf: false | ||
|
|
||
| testpackage: | ||
| skip-regexp: internal_test\.go | ||
|
|
||
| varnamelen: | ||
| max-distance: 10 | ||
|
|
||
| whitespace: | ||
| multi-if: true | ||
| multi-func: true | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Key name should be linters-settings (not settings)
Per config schema, per-linter options live under linters-settings at the top level, not under linters.settings.
Minimal fix:
- settings:
+linters-settings:
cyclop:
max-complexity: 25
depguard:
rules:
main:
files:
- $all
- "!$test"
- "!**/examples/**/*"
allow:
- $gostd
- gopkg.in/yaml.v3
- github.com/Masterminds/sprig/v3
test:
files:
- $test
- "**/examples/**/*"
allow:
- $gostd
- gopkg.in/yaml.v3
- github.com/Masterminds/sprig/v3
- github.com/stretchr/testify/assert
- github.com/AlphaOne1/dmorph
- modernc.org/sqlite
exhaustive:
default-signifies-exhaustive: true
mnd:
ignored-numbers:
- "2"
paralleltest:
ignore-missing: true
perfsprint:
errorf: false
testpackage:
skip-regexp: internal_test\.go
varnamelen:
max-distance: 10
whitespace:
multi-if: true
multi-func: trueTo ensure no regressions, validate the config with the linter CLI: golangci-lint run -v (or golangci-lint linters to inspect the parsed config).
Replace settings with linters-settings in .golangci.yaml
Per the official golangci-lint schema, per-linter options belong under the top-level linters-settings key—not settings.
Please update your config (in .golangci.yaml, lines 43–92) as follows:
- settings:
+ linters-settings:
cyclop:
max-complexity: 25
depguard:
rules:
main:
…After making this change, re-run golangci-lint run -v (or golangci-lint linters) to confirm the config parses correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| settings: | |
| cyclop: | |
| max-complexity: 25 | |
| depguard: | |
| rules: | |
| main: | |
| files: | |
| - $all | |
| - "!$test" | |
| - "!**/examples/**/*" | |
| allow: | |
| - $gostd | |
| - gopkg.in/yaml.v3 | |
| - github.com/Masterminds/sprig/v3 | |
| test: | |
| files: | |
| - $test | |
| - "**/examples/**/*" | |
| allow: | |
| - $gostd | |
| - gopkg.in/yaml.v3 | |
| - github.com/Masterminds/sprig/v3 | |
| - github.com/stretchr/testify/assert | |
| - github.com/AlphaOne1/dmorph | |
| - modernc.org/sqlite | |
| exhaustive: | |
| default-signifies-exhaustive: true | |
| mnd: | |
| ignored-numbers: | |
| - "2" | |
| paralleltest: | |
| ignore-missing: true | |
| perfsprint: | |
| errorf: false | |
| testpackage: | |
| skip-regexp: internal_test\.go | |
| varnamelen: | |
| max-distance: 10 | |
| whitespace: | |
| multi-if: true | |
| multi-func: true | |
| linters-settings: | |
| cyclop: | |
| max-complexity: 25 | |
| depguard: | |
| rules: | |
| main: | |
| files: | |
| - $all | |
| - "!$test" | |
| - "!**/examples/**/*" | |
| allow: | |
| - $gostd | |
| - gopkg.in/yaml.v3 | |
| - github.com/Masterminds/sprig/v3 | |
| test: | |
| files: | |
| - $test | |
| - "**/examples/**/*" | |
| allow: | |
| - $gostd | |
| - gopkg.in/yaml.v3 | |
| - github.com/Masterminds/sprig/v3 | |
| - github.com/stretchr/testify/assert | |
| - github.com/AlphaOne1/dmorph | |
| - modernc.org/sqlite | |
| exhaustive: | |
| default-signifies-exhaustive: true | |
| mnd: | |
| ignored-numbers: | |
| - "2" | |
| paralleltest: | |
| ignore-missing: true | |
| perfsprint: | |
| errorf: false | |
| testpackage: | |
| skip-regexp: internal_test\.go | |
| varnamelen: | |
| max-distance: 10 | |
| whitespace: | |
| multi-if: true | |
| multi-func: true |
🤖 Prompt for AI Agents
In .golangci.yaml between lines 43 and 92, replace the top-level key "settings"
with "linters-settings" to correctly specify per-linter options according to the
golangci-lint schema. This involves renaming the "settings:" key to
"linters-settings:" while keeping the nested configuration unchanged. After
updating, run golangci-lint to verify the configuration parses without errors.
| var ( | ||
| TapplyStepsStream = applyStepsStream | ||
| TmigrationFromFileFS = migrationFromFileFS | ||
| TmigrationOrder = migrationOrder | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace global var aliases with thin wrapper functions to satisfy gochecknoglobals
Global variables trigger gochecknoglobals. Use wrappers instead; behavior unchanged and constrained to _test.go files.
Apply this diff:
-package dmorph
-
-import "database/sql"
-
-// The exported names in this file are only used for internal testing and are not part of the public API.
-
-var (
- TapplyStepsStream = applyStepsStream
- TmigrationFromFileFS = migrationFromFileFS
- TmigrationOrder = migrationOrder
-)
-
-func (m *Morpher) TapplyMigrations(db *sql.DB, lastMigration string) error {
- return m.applyMigrations(db, lastMigration)
-}
+package dmorph
+
+import (
+ "database/sql"
+ "io"
+ "io/fs"
+ "log/slog"
+)
+
+// The exported names in this file are only used for internal testing and are not part of the public API.
+
+func TapplyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error {
+ return applyStepsStream(tx, r, id, log)
+}
+
+func TmigrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration {
+ return migrationFromFileFS(name, dir, log)
+}
+
+func TmigrationOrder(m, n Migration) int {
+ return migrationOrder(m, n)
+}
+
+func (m *Morpher) TapplyMigrations(db *sql.DB, lastMigration string) error {
+ return m.applyMigrations(db, lastMigration)
+}🧰 Tools
🪛 golangci-lint (2.2.2)
8-8: TapplyStepsStream is a global variable
(gochecknoglobals)
9-9: TmigrationFromFileFS is a global variable
(gochecknoglobals)
10-10: TmigrationOrder is a global variable
(gochecknoglobals)
🤖 Prompt for AI Agents
In exports_internal_test.go around lines 7 to 11, replace the global variable
aliases with thin wrapper functions that return the original variables. This
avoids global variables triggering gochecknoglobals while preserving the same
behavior. Define functions named TapplyStepsStream, TmigrationFromFileFS, and
TmigrationOrder that return the respective original variables instead of using
var aliases.
- Simplified `for` loop in `migration.go` using `range`. - Replaced `assert` with `require` in tests for stricter checks. - Updated scanner buffer handling in `applyStepsStream` for performance. - Adjusted `.golangci.yaml` linter rules to include `require`. - Minor refactoring in `dialects.go` to inline error handling. - Clarified comments in `codeql.yml`.
Codecov Report❌ Patch coverage is
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
file_migration.go (1)
123-128: Bug: leading comments are not removed as documentedThe code writes the line to the buffer before checking for a leading "--" comment. As a result, comments are kept, contradicting the updated comment text. Move the write after the comment check.
Apply this diff:
- buf.Write(scanner.Bytes()) - - if newStep && strings.HasPrefix(scanner.Text(), "--") { - // skip leading comments - continue - } + if newStep && strings.HasPrefix(scanner.Text(), "--") { + // skip leading comments + continue + } + + buf.Write(scanner.Bytes())
🧹 Nitpick comments (2)
migration_test.go (2)
63-65: Consider addressing the type assertion warning.The static analysis tool flags unsafe type assertions that could panic if the assertion fails.
Consider using the two-value form for safer type assertion:
- dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFSOr handle it explicitly:
+ readDirFS, ok := migrationsDir.(fs.ReadDirFS) + require.True(t, ok, "migrationsDir must implement fs.ReadDirFS") runErr := dmorph.Run(db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS(readDirFS))Also applies to: 98-100
472-472: Consider extracting the repeated test string as a constant.The string "utter nonsense" appears multiple times in the test file.
Consider defining a constant at the package level:
+const invalidSQLTemplate = "utter nonsense" + func TestMigrationRunInvalidCreate(t *testing.T) { // ... dialect := dmorph.DialectSQLite() - dialect.CreateTemplate = "utter nonsense" + dialect.CreateTemplate = invalidSQLTemplateAnd use it in lines 504 and 573 as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/codeql.yml(1 hunks).golangci.yaml(2 hunks)dialects.go(2 hunks)dialects_test.go(5 hunks)file_migration.go(4 hunks)migration.go(9 hunks)migration_test.go(23 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/codeql.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .golangci.yaml
🧰 Additional context used
🧬 Code Graph Analysis (3)
dialects_test.go (2)
dialects.go (1)
BaseDialect(15-19)dialect_sqlite.go (1)
DialectSQLite(7-22)
migration_test.go (5)
migration.go (16)
Run(294-302)WithDialect(84-90)WithMigrations(93-99)ErrMigrationsTooOld(45-45)ErrMigrationsUnrelated(24-24)ErrMigrationsUnsorted(28-28)Migration(55-58)Morpher(73-78)Dialect(48-52)ErrNoDialect(31-31)ErrNoMigrations(34-34)ErrNoMigrationTable(37-37)ErrMigrationTableNameInvalid(40-40)NewMorpher(132-149)WithLog(103-109)WithTableName(113-127)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(68-85)WithMigrationFromFileFS(58-64)FileMigration(18-22)WithMigrationFromFile(35-54)exports_internal_test.go (1)
TmigrationOrder(10-10)dialects.go (1)
BaseDialect(15-19)
file_migration.go (1)
file_migration_test.go (2)
TestMigrationFromFileFSError(67-77)TestWithMigrationFromFileError(41-64)
🪛 golangci-lint (2.2.2)
dialects.go
29-29: (*database/sql.Tx).Exec must not be called. use (*database/sql.Tx).ExecContext
(noctx)
migration_test.go
63-63: right hand must be only type assertion
(forcetypeassert)
98-98: right hand must be only type assertion
(forcetypeassert)
96-96: require-error: for error assertions use require
(testifylint)
169-169: right hand must be only type assertion
(forcetypeassert)
204-204: right hand must be only type assertion
(forcetypeassert)
208-208: require-error: for error assertions use require
(testifylint)
243-243: require-error: for error assertions use require
(testifylint)
284-284: right hand must be only type assertion
(forcetypeassert)
472-472: string utter nonsense has 3 occurrences, make it a constant
(goconst)
file_migration.go
111-111: parameter name 'tx' is too short for the scope of its usage
(varnamelen)
🔇 Additional comments (18)
dialects.go (1)
13-14: LGTM! Documentation improvements and new field addition look good.The grammar fix in the comment and the addition of the
RegisterTemplatefield are appropriate changes that enhance the BaseDialect struct.dialects_test.go (2)
4-14: Good migration to external test package.Moving the test to
dmorph_testand using fully qualified references is a good practice for testing the public API.
119-120: LGTM! Documentation improvements are clear.The comment split improves readability while maintaining clarity about the test's purpose.
file_migration.go (5)
75-75: Good formatting improvement for log readability.Adding a blank line after the log statement improves code readability.
87-87: LGTM! Grammar correction in comment.The article change from "an" to "a" is grammatically correct.
109-110: Good documentation improvements for comment handling.The updated comments provide clearer guidance about leading vs in-line comments and appropriate hyphenation.
112-118: Good addition of scanner buffer configuration.The introduction of constants for buffer sizes and configuring the scanner with a larger buffer is a good improvement that will help handle larger migration files without buffer overflow issues.
146-146: Good formatting improvement in comment.Wrapping the semicolon in backticks improves the documentation clarity.
migration.go (5)
22-44: LGTM! Documentation improvements enhance clarity.The improvements to error variable comments, including better punctuation and line wrapping, make the documentation more professional and readable.
83-88: Good formatting consistency across option functions.Adding blank lines before return statements improves code readability and maintains consistency across all option functions.
Also applies to: 92-97, 103-107, 113-125
232-234: Good documentation improvement for transaction safety.Splitting the rollback safeguard comment into two sentences improves clarity about the purpose of the deferred rollback.
220-221: Consistent formatting improvements throughout the file.The addition of blank lines improves code readability and maintains consistency across error handling paths and function endings.
Also applies to: 239-240, 245-246, 251-252, 259-260, 268-269, 288-289
278-279: Good improvements to comments and loop syntax.The grammar improvement from "amount" to "number" is more precise, and using
for i := rangeis cleaner Go syntax.Also applies to: 283-284
migration_test.go (5)
4-18: Good migration to external test package.Converting to
dmorph_testpackage and using fully qualified references is consistent with the test refactoring pattern across the codebase.
110-111: Good formatting improvement.Adding a blank line after the Exec call improves code readability.
147-147: Good documentation improvements for test functions.The updated comments provide clearer descriptions of what each test is validating.
Also applies to: 182-182, 217-217, 581-582
319-319: Good use of exported test wrapper.Using
dmorph.TmigrationOrderto test the internal migration ordering function is a clean approach.
542-542: Good use of the new test wrapper method.Using
TapplyMigrationsmethod provides controlled access to the internalapplyMigrationsfunction for testing purposes.Also applies to: 577-577, 631-631
- Replaced `assert` with `require` in tests for stricter validation. - Updated migration directory handling with explicit safety comments. - Adjusted buffer writes to fix potential order issues in `applyStepsStream`. - Added safety net with deferred transaction rollback in `dialects.go`.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dialects.go (1)
29-34: Panic-safe deferred rollback is correct and robust (thanks for integrating it).The defer guard plus setting tx = nil in error/success paths avoids double rollbacks and leaks on panics. This is the right trade-off with explicit rollback/commit handling.
Also applies to: 38-38, 45-45, 50-50
file_migration.go (1)
122-131: Leading comment skip fixed (write moved after the check).Reordering the write after the “--” check now actually removes leading comments as documented.
🧹 Nitpick comments (5)
dialects.go (1)
36-36: noctx lint: annotate Exec to match team preference (avoid ExecContext).Per the repo preference to keep code simple without context plumbing, add a nolint to satisfy golangci-lint’s noctx rule.
Apply this diff:
- if _, execErr := tx.Exec(fmt.Sprintf(b.CreateTemplate, tableName)); execErr != nil { + if _, execErr := tx.Exec(fmt.Sprintf(b.CreateTemplate, tableName)); execErr != nil { //nolint:noctx // team prefers no context plumbing herefile_migration.go (2)
122-137: Handle whitespace-only semicolon lines and avoid buffering the delimiter.Two small robustness tweaks:
- Treat lines with only whitespace+semicolon as a delimiter (e.g., " ;").
- Don’t append the semicolon line into the SQL buffer before Exec. Many drivers accept trailing “;”, but not appending it avoids surprises.
Apply this diff:
- for i = 0; scanner.Scan(); { - if newStep && strings.HasPrefix(scanner.Text(), "--") { + for i = 0; scanner.Scan(); { + line := scanner.Text() + trimmed := strings.TrimSpace(line) + + if newStep && strings.HasPrefix(trimmed, "--") { // skip leading comments continue } - newStep = false - - buf.Write(scanner.Bytes()) - - if scanner.Text() == ";" { + // Step delimiter on a line containing only `;` (allow surrounding whitespace) + if trimmed == ";" { log.Info("migration step", slog.String("id", id), slog.Int("step", i), ) if _, err := tx.Exec(buf.String()); err != nil { return err } buf.Reset() i++ + newStep = true + continue } + + // Append the current line (preserve formatting by adding a newline between lines) + if buf.Len() > 0 { + _ = buf.WriteByte('\n') + } + buf.WriteString(line) + newStep = false
119-121: Rename loop counter ‘i’ to ‘step’ for clarity.Improves readability and satisfies varnamelen if you keep it enabled.
Apply this diff:
- newStep := true - var i int + newStep := true + var step int @@ - for i = 0; scanner.Scan(); { + for step = 0; scanner.Scan(); { @@ - slog.Int("step", i), + slog.Int("step", step), ) @@ - i++ + step++ @@ - slog.Int("step", i), + slog.Int("step", step),Also applies to: 133-136, 148-151
migration_test.go (2)
611-613: Name clarity: rename ‘d’ to ‘baseDialect’.Improves readability and satisfies varnamelen.
Apply this diff:
- d, dialectOK := morpher.Dialect.(dmorph.BaseDialect) + baseDialect, dialectOK := morpher.Dialect.(dmorph.BaseDialect) require.True(t, dialectOK, "dialect is not a BaseDialect") @@ - morpher.Dialect = d + morpher.Dialect = baseDialectAlso applies to: 630-631
473-479: Deduplicate “utter nonsense” with a test-local constant.This string is used multiple times; a constant improves clarity and satisfies goconst.
Apply these diffs and add a constant near the top of the file (see snippet below):
- dialect := dmorph.DialectSQLite() - dialect.CreateTemplate = "utter nonsense" + dialect := dmorph.DialectSQLite() + dialect.CreateTemplate = nonsense- dialect := dmorph.DialectSQLite() - dialect.AppliedTemplate = "utter nonsense" + dialect := dmorph.DialectSQLite() + dialect.AppliedTemplate = nonsenseAdditionally update the similar assignment near line 575:
- baseDialect.RegisterTemplate = nonsense
Add this constant near the imports:
const nonsense = "utter nonsense"Also applies to: 505-511
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dialects.go(2 hunks)file_migration.go(4 hunks)migration_test.go(23 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:21:31.632Z
Learnt from: AlphaOne1
PR: AlphaOne1/dmorph#31
File: dialects.go:29-29
Timestamp: 2025-08-12T20:21:31.632Z
Learning: In the dmorph codebase, the maintainer AlphaOne1 prefers to avoid using context.Background() and context handling in database operations to reduce code complexity and clutter, even when suggested by linters like noctx.
Applied to files:
dialects.go
🧬 Code Graph Analysis (3)
dialects.go (3)
migration.go (4)
Dialect(73-78)EnsureMigrationTableExists(48-52)m(148-166)WithDialect(84-89)dialects_test.go (1)
TestDialectStatements(17-59)dialect_mysql.go (1)
DialectMySQL(7-16)
file_migration.go (2)
file_migration_test.go (2)
TestMigrationFromFileFSError(67-77)TestApplyStepsStreamError(80-122)migration.go (1)
m(173-206)
migration_test.go (5)
migration.go (16)
Run(294-302)WithDialect(84-90)WithMigrations(93-99)ErrMigrationsUnrelated(24-24)ErrMigrationsUnsorted(28-28)Migration(55-58)Morpher(73-78)Dialect(48-52)ErrNoDialect(31-31)ErrNoMigrations(34-34)ErrNoMigrationTable(37-37)ErrMigrationTableNameInvalid(40-40)NewMorpher(132-149)WithLog(103-109)WithTableName(113-127)m(173-206)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(68-85)WithMigrationFromFileFS(58-64)FileMigration(18-22)WithMigrationFromFile(35-54)exports_internal_test.go (1)
TmigrationOrder(10-10)dialects.go (1)
BaseDialect(15-19)
🪛 golangci-lint (2.2.2)
dialects.go
36-36: (*database/sql.Tx).Exec must not be called. use (*database/sql.Tx).ExecContext
(noctx)
file_migration.go
111-111: parameter name 'id' is too short for the scope of its usage
(varnamelen)
120-120: variable name 'i' is too short for the scope of its usage
(varnamelen)
migration_test.go
63-63: right hand must be only type assertion
(forcetypeassert)
99-99: right hand must be only type assertion
(forcetypeassert)
171-171: right hand must be only type assertion
(forcetypeassert)
206-206: right hand must be only type assertion
(forcetypeassert)
210-210: require-error: for error assertions use require
(testifylint)
245-245: require-error: for error assertions use require
(testifylint)
286-286: right hand must be only type assertion
(forcetypeassert)
474-474: string utter nonsense has 3 occurrences, make it a constant
(goconst)
611-611: variable name 'd' is too short for the scope of its usage
(varnamelen)
🔇 Additional comments (1)
file_migration.go (1)
112-119: Scanner buffer configuration: good defensive increase for large statements.Using a 64 KiB initial and 1 MiB max token size prevents silent token truncation on long lines.
- Updated all migration methods to include `context.Context` as the first argument. - Refactored related tests to use `context.Background()` where necessary. - Adjusted `applyStepsStream` and SQL execution calls to utilize context-aware methods. - Improved function signatures in `dialects.go` and `file_migration.go` for better consistency with Go best practices.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
exports_internal_test.go (1)
10-14: Consider using wrapper functions instead of global variable aliasesThe global variable aliases trigger the gochecknoglobals linter. While this approach works for internal testing, using thin wrapper functions would be cleaner and avoid linter warnings.
Apply this diff to replace global variables with wrapper functions:
-var ( - TapplyStepsStream = applyStepsStream - TmigrationFromFileFS = migrationFromFileFS - TmigrationOrder = migrationOrder -) +func TapplyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error { + return applyStepsStream(ctx, tx, r, id, log) +} + +func TmigrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration { + return migrationFromFileFS(name, dir, log) +} + +func TmigrationOrder(m, n Migration) int { + return migrationOrder(m, n) +}You'll also need to add the necessary imports:
import ( "context" "database/sql" + "io" + "io/fs" + "log/slog" )file_migration_test.go (1)
74-78: Consider using os.DirFS for better portabilityThe
os.OpenRootfunction with.FS()method is less common and may have compatibility concerns. Usingos.DirFSwould be simpler and more portable.Apply this diff to simplify the directory access:
- dir, dirErr := os.OpenRoot("testData") - - assert.NoError(t, dirErr, "could not open test data directory") - - mig := dmorph.TmigrationFromFileFS("nonexistent", dir.FS(), slog.Default()) + fsys := os.DirFS("testData") + mig := dmorph.TmigrationFromFileFS("nonexistent", fsys, slog.Default())Note:
os.DirFShas been available since Go 1.16 and removes the need for explicit error handling when opening a local directory.migration_test.go (5)
60-68: Fix panic-prone forced type assertions inside calls.The forced type assertion
migrationsDir.(fs.ReadDirFS)on line 68 can panic if the assertion fails. Assert the type once before using it.Apply this diff:
migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") require.NoError(t, migrationsDirErr, "migrations directory could not be opened") +rd, ok := migrationsDir.(fs.ReadDirFS) +require.True(t, ok, "migrationsDir does not implement fs.ReadDirFS") + runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(rd))
95-106: Fix panic-prone forced type assertion inside call.The forced type assertion on line 106 can panic if the assertion fails.
Apply this diff:
+rd, ok := migrationsDir.(fs.ReadDirFS) +require.True(t, ok, "migrationsDir does not implement fs.ReadDirFS") + runErr = dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(rd))
176-179: Fix panic-prone forced type assertion inside call.The forced type assertion on line 179 can panic if the assertion fails.
Apply this diff:
require.NoError(t, migrationsDirErr, "migrations directory could not be opened") +rd, ok := migrationsDir.(fs.ReadDirFS) +require.True(t, ok, "migrationsDir does not implement fs.ReadDirFS") + runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS(rd))
213-216: Fix panic-prone forced type assertion inside call.The forced type assertion on line 216 can panic if the assertion fails.
Apply this diff:
+rd, ok := migrationsDir.(fs.ReadDirFS) +require.True(t, ok, "migrationsDir does not implement fs.ReadDirFS") + runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS(rd))
297-300: Fix panic-prone forced type assertion inside call.The forced type assertion on line 300 can panic if the assertion fails.
Apply this diff:
+rd, ok := migrationsDir.(fs.ReadDirFS) +require.True(t, ok, "migrationsDir does not implement fs.ReadDirFS") + runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS(rd))
🧹 Nitpick comments (6)
migration_test.go (5)
100-100: Userequirefor error assertions.For consistency with other tests and better test failure reporting.
Apply this diff:
-assert.NoError(t, runErr, "preparation migrations could not be run") +require.NoError(t, runErr, "preparation migrations could not be run")
255-255: Userequirefor error assertions.For consistency with other tests and better test failure reporting.
Apply this diff:
-assert.NoError(t, runErr, "preparation migrations could not be run") +require.NoError(t, runErr, "preparation migrations could not be run")
486-486: Consider extracting repeated string literal as a constant.The string
"utter nonsense"appears multiple times in the test file (lines 486, 518, 587). Consider extracting it as a constant for better maintainability.Add at the top of the file after other declarations:
const invalidSQLStatement = "utter nonsense"Then replace all occurrences:
-dialect.CreateTemplate = "utter nonsense" +dialect.CreateTemplate = invalidSQLStatement
492-492: Userequirefor error assertions.For consistency with other tests and better test failure reporting.
Apply this diff:
-assert.NoError(t, morpherErr, "morpher could not be created") +require.NoError(t, morpherErr, "morpher could not be created")
524-524: Userequirefor error assertions.For consistency with other tests and better test failure reporting.
Apply this diff:
-assert.NoError(t, morpherErr, "morpher could not be created") +require.NoError(t, morpherErr, "morpher could not be created")migration.go (1)
227-236: Consider adding context check before starting transaction.Before beginning a transaction, it's good practice to check if the context is already cancelled to avoid unnecessary database operations.
Apply this diff:
m.Log.Info("applying migration", slog.String("file", migration.Key())) startMigration = time.Now() + +// Check context before starting transaction +if err := ctx.Err(); err != nil { + return fmt.Errorf("context cancelled before migration %s: %w", migration.Key(), err) +} + tx, txBeginErr := db.Begin()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dialects.go(2 hunks)dialects_test.go(5 hunks)exports_internal_test.go(1 hunks)file_migration.go(7 hunks)file_migration_test.go(5 hunks)migration.go(11 hunks)migration_test.go(22 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:21:31.632Z
Learnt from: AlphaOne1
PR: AlphaOne1/dmorph#31
File: dialects.go:29-29
Timestamp: 2025-08-12T20:21:31.632Z
Learning: In the dmorph codebase, the maintainer AlphaOne1 prefers to avoid using context.Background() and context handling in database operations to reduce code complexity and clutter, even when suggested by linters like noctx.
Applied to files:
dialects.gomigration.gomigration_test.go
🧬 Code Graph Analysis (5)
exports_internal_test.go (1)
migration.go (1)
Morpher(74-79)
file_migration_test.go (4)
migration.go (2)
Run(295-303)WithDialect(85-91)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (1)
WithMigrationFromFile(36-55)exports_internal_test.go (2)
TmigrationFromFileFS(12-12)TapplyStepsStream(11-11)
file_migration.go (1)
migration.go (2)
MorphOption(82-82)Morpher(74-79)
dialects_test.go (8)
dialects.go (1)
BaseDialect(16-20)dialect_csvq.go (1)
DialectCSVQ(7-22)dialect_db2.go (1)
DialectDB2(7-31)dialect_mssql.go (1)
DialectMSSQL(7-27)dialect_mysql.go (1)
DialectMySQL(7-16)dialect_oracle.go (1)
DialectOracle(7-31)dialect_postgres.go (1)
DialectPostgres(7-22)dialect_sqlite.go (1)
DialectSQLite(7-22)
migration_test.go (5)
migration.go (13)
Run(295-303)WithDialect(85-91)WithMigrations(94-100)ErrMigrationsTooOld(46-46)ErrMigrationsUnrelated(25-25)ErrMigrationsUnsorted(29-29)Migration(56-59)Morpher(74-79)Dialect(49-53)ErrMigrationTableNameInvalid(41-41)NewMorpher(133-150)WithLog(104-110)WithTableName(114-128)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(69-86)WithMigrationFromFileFS(59-65)FileMigration(19-23)WithMigrationFromFile(36-55)exports_internal_test.go (1)
TmigrationOrder(13-13)dialects.go (1)
BaseDialect(16-20)
🪛 golangci-lint (2.2.2)
exports_internal_test.go
11-11: TapplyStepsStream is a global variable
(gochecknoglobals)
12-12: TmigrationFromFileFS is a global variable
(gochecknoglobals)
13-13: TmigrationOrder is a global variable
(gochecknoglobals)
dialects.go
24-24: variable name 'tx' is too short for the scope of its usage
(varnamelen)
27-27: error returned from external package is unwrapped: sig: func (*database/sql.DB).BeginTx(ctx context.Context, opts *database/sql.TxOptions) (*database/sql.Tx, error)
(wrapcheck)
file_migration_test.go
76-76: require-error: for error assertions use require
(testifylint)
file_migration.go
112-112: parameter name 'tx' is too short for the scope of its usage
(varnamelen)
136-136: error returned from external package is unwrapped: sig: func (*database/sql.Tx).ExecContext(ctx context.Context, query string, args ...any) (database/sql.Result, error)
(wrapcheck)
migration.go
178-178: parameter name 'db' is too short for the scope of its usage
(varnamelen)
215-215: parameter name 'db' is too short for the scope of its usage
(varnamelen)
migration_test.go
64-64: right hand must be only type assertion
(forcetypeassert)
102-102: right hand must be only type assertion
(forcetypeassert)
100-100: require-error: for error assertions use require
(testifylint)
176-176: right hand must be only type assertion
(forcetypeassert)
213-213: right hand must be only type assertion
(forcetypeassert)
255-255: require-error: for error assertions use require
(testifylint)
297-297: right hand must be only type assertion
(forcetypeassert)
486-486: string utter nonsense has 3 occurrences, make it a constant
(goconst)
492-492: require-error: for error assertions use require
(testifylint)
524-524: require-error: for error assertions use require
(testifylint)
620-620: (*database/sql.DB).Exec must not be called. use (*database/sql.DB).ExecContext
(noctx)
🔇 Additional comments (5)
dialects_test.go (1)
93-99: Typo in SQL template will cause test failureThe SQL template has "CRATE TABLE" instead of "CREATE TABLE", which will cause an SQL syntax error as intended for the test.
dialects.go (2)
23-54: Excellent implementation of context propagation and transaction safetyThe updated implementation properly handles context throughout the transaction lifecycle and includes a robust safety net for unexpected panics. The pattern of setting
tx = nilafter explicit rollback/commit prevents double rollback attempts.
57-77: Good context propagation in query operationsThe method correctly uses
QueryContextfor context-aware database queries and maintains proper error handling witherrors.Join.file_migration.go (1)
112-168: Well-structured migration step processing with proper context handlingThe implementation correctly:
- Uses
ExecContextfor all database operations- Handles leading comment removal properly by checking before writing to buffer
- Preserves SQL formatting with newline handling
- Processes the final statement without trailing semicolon
- Provides clear step-based logging
The buffer size constants (64KB initial, 1MB max) are reasonable for typical migration files.
migration.go (1)
1-303: Excellent context propagation implementation!The addition of context support throughout the migration system is well-implemented and follows Go best practices. The changes enable proper cancellation and timeout support, which is essential for production database operations.
…lidation - Added context cancellation check before starting a database transaction in `migration.go`. - Replaced `assert` with `require` in tests for stricter and immediate validation. - Added safety comments for migration directory handling in tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
file_migration_test.go (1)
75-83: Prefer os.DirFS over os.OpenRoot(...).FS() for portability.DirFS("testData") is simpler and works across Go versions; avoids opening a directory handle and the extra require. This was suggested earlier as well.
Apply this diff:
-dir, dirErr := os.OpenRoot("testData") - -require.NoError(t, dirErr, "could not open test data directory") - -mig := dmorph.TmigrationFromFileFS("nonexistent", dir.FS(), slog.Default()) +fsys := os.DirFS("testData") +mig := dmorph.TmigrationFromFileFS("nonexistent", fsys, slog.Default())migration_test.go (3)
292-299: Use ExecContext for DB ops in tests.Use db.ExecContext with a ctx to align with the ctx-aware pattern and appease noctx. This was flagged earlier as well.
- _, execErr := db.Exec(` + _, execErr := db.ExecContext(context.Background(), ` INSERT INTO migrations (id, create_ts) VALUES ('01_base_table', '2021-01-02 00:00:00'); INSERT INTO migrations (id, create_ts) VALUES ('02_addon_table', '2021-01-01 00:00:00'); `)
623-624: Switch to ExecContext for PRAGMA.As previously noted, use ExecContext to avoid noctx and mirror production behavior.
- _, execErr := db.Exec("PRAGMA foreign_keys = ON") + _, execErr := db.ExecContext(context.Background(), "PRAGMA foreign_keys = ON") require.NoError(t, execErr, "foreign keys checking could not be enabled")
64-69: Avoid forced type assertions that can panic; assert once and reuse.The migrationsDir.(fs.ReadDirFS) assertions will panic if the underlying FS changes. Use the comma-ok idiom and require.True before passing to WithMigrationsFromFS.
Apply this shape at each occurrence:
- runErr := dmorph.Run(context.Background(), + rd, ok := migrationsDir.(fs.ReadDirFS) + require.True(t, ok, "migrationsDir does not implement fs.ReadDirFS") + + runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(rd))Also applies to: 102-107, 176-181, 214-219, 299-304
🧹 Nitpick comments (9)
migration.go (4)
239-243: Avoid stacking defers inside a loop; scope the transaction to a helper.Deferring tx.Rollback() in a loop accumulates defers and attempts a rollback after commit (ErrTxDone), which is benign but noisy and can obscure logic. Encapsulate a single migration in a helper so the defer’s lifetime is limited to one iteration.
For example (outside this hunk):
func (m *Morpher) runOneMigration(ctx context.Context, db *sql.DB, mig Migration) error { tx, err := db.BeginTx(ctx, nil) if err != nil { return fmt.Errorf("begin tx: %w", err) } defer func() { _ = tx.Rollback() }() if err := mig.Migrate(ctx, tx); err != nil { return err } if err := m.Dialect.RegisterMigration(ctx, tx, mig.Key(), m.TableName); err != nil { return err } return tx.Commit() }Then in applyMigrations: call m.runOneMigration(ctx, db, migration).
116-119: Return a typed error for empty table names for consistency.Elsewhere you use sentinel errors; returning fmt.Errorf here is inconsistent and harder to assert in tests. Prefer a typed error.
Apply this diff:
- if len(tableName) < 1 { - return fmt.Errorf("table name empty") - } + if len(tableName) < 1 { + return ErrNoMigrationTable + }
23-23: Tiny doc polish: “signals/indicates” reads better than “signalizes/tells”.Purely wording; improves readability of exported error docs.
-// ErrMigrationsUnrelated signalizes that the set of migrations to apply and the already applied set do not have the +// ErrMigrationsUnrelated signals that the set of migrations to apply and the already applied set do not have the @@ -// ErrMigrationsUnsorted tells that the already applied migrations were not registered in the order +// ErrMigrationsUnsorted indicates that the already applied migrations were not registered in the order @@ -// ErrNoMigrations signalizes that no migrations were chosen to be applied. +// ErrNoMigrations indicates that no migrations were chosen to be applied.Also applies to: 27-28, 34-34
178-179: About varnamelen on “db”: prefer linter config over renaming.“db” is idiomatic Go for *sql.DB. I’d suppress varnamelen for params like ctx, db, tx in .golangci.yml instead of renaming.
Would you like a PR-ready .golangci.yml snippet to disable varnamelen for these identifiers?
file_migration_test.go (2)
107-112: Use BeginTx with context in tests for consistency.Tests now exercise ctx-aware flow; use BeginTx to be consistent and future-proof if you add deadlines/timeouts to tests.
-tx, txErr := db.Begin() +tx, txErr := db.BeginTx(context.Background(), nil) @@ -tx, txErr = db.Begin() +tx, txErr = db.BeginTx(context.Background(), nil)Also applies to: 117-121
38-42: Optional: consider using a test-scoped ctx with timeout.Using context.Background() is fine, but a test ctx with timeout can help catch hangs and validate cancellation behavior.
If useful, I can sketch a small helper returning ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second).
Also applies to: 63-67, 81-82, 111-111, 124-125
migration_test.go (3)
489-496: DRY: extract repeated “utter nonsense” into a test constant.This string appears multiple times; factoring it reduces noise and satisfies goconst.
Add near the imports (outside this hunk):
const nonsense = "utter nonsense"Then update usages:
- dialect.CreateTemplate = "utter nonsense" + dialect.CreateTemplate = nonsense @@ - dialect.AppliedTemplate = "utter nonsense" + dialect.AppliedTemplate = nonsenseAlso applies to: 520-528
25-40: Optional: extract prepareDB into a shared test helper.Both dmorph_test packages define prepareDB; consider a single helper (e.g., helpers_test.go) to reduce duplication.
If helpful, I can draft a helpers_test.go with prepareDB(t *testing.T) that uses t.Cleanup to remove the file automatically.
60-70: Architecture: consider changing WithMigrationsFromFS to accept fs.FS.The forced ReadDirFS assertions in tests stem from API shape. If WithMigrationsFromFS accepted fs.FS and internally used fs.ReadDir(d, ".") you could avoid type assertions entirely, making the API friendlier to any FS implementing only fs.FS.
Happy to sketch a backward-compatible overload or a new WithMigrationsFromFS2 accepting fs.FS.
Also applies to: 91-101, 172-182, 210-220, 286-296
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
file_migration_test.go(5 hunks)migration.go(10 hunks)migration_test.go(22 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:21:31.662Z
Learnt from: AlphaOne1
PR: AlphaOne1/dmorph#31
File: dialects.go:29-29
Timestamp: 2025-08-12T20:21:31.662Z
Learning: In the dmorph codebase, the maintainer AlphaOne1 prefers to avoid using context.Background() and context handling in database operations to reduce code complexity and clutter, even when suggested by linters like noctx.
Applied to files:
migration.gomigration_test.go
🧬 Code graph analysis (2)
migration_test.go (5)
migration.go (13)
Run(301-309)WithDialect(85-91)WithMigrations(94-100)Migration(56-59)Morpher(74-79)Dialect(49-53)ErrNoDialect(32-32)ErrNoMigrations(35-35)ErrNoMigrationTable(38-38)ErrMigrationTableNameInvalid(41-41)NewMorpher(133-150)WithLog(104-110)WithTableName(114-128)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(69-86)WithMigrationFromFileFS(59-65)FileMigration(19-23)WithMigrationFromFile(36-55)exports_internal_test.go (1)
TmigrationOrder(13-13)dialects.go (1)
BaseDialect(16-20)
file_migration_test.go (4)
migration.go (2)
Run(301-309)WithDialect(85-91)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (1)
WithMigrationFromFile(36-55)exports_internal_test.go (2)
TmigrationFromFileFS(12-12)TapplyStepsStream(11-11)
🪛 golangci-lint (2.2.2)
migration.go
178-178: parameter name 'db' is too short for the scope of its usage
(varnamelen)
233-233: (*database/sql.DB).Begin must not be called. use (*database/sql.DB).BeginTx
(noctx)
215-215: parameter name 'db' is too short for the scope of its usage
(varnamelen)
236-236: error returned from external package is unwrapped: sig: func (*database/sql.DB).Begin() (*database/sql.Tx, error)
(wrapcheck)
migration_test.go
64-64: right hand must be only type assertion
(forcetypeassert)
102-102: right hand must be only type assertion
(forcetypeassert)
176-176: right hand must be only type assertion
(forcetypeassert)
214-214: right hand must be only type assertion
(forcetypeassert)
299-299: right hand must be only type assertion
(forcetypeassert)
489-489: string utter nonsense has 3 occurrences, make it a constant
(goconst)
623-623: (*database/sql.DB).Exec must not be called. use (*database/sql.DB).ExecContext
(noctx)
🔇 Additional comments (6)
migration.go (1)
48-53: Good move: context-aware API across Dialect/Migration and Run.Threading ctx through the public surface and internal flow is coherent and enables cancellation/timeouts end-to-end. This aligns well with the test/package split and the new wrappers. One note: earlier learnings mention a preference to avoid context.Background in DB ops; assuming this PR marks a directional change toward ctx-first. Please confirm.
We previously learned you prefer to avoid context.Background to reduce clutter. Is adopting ctx across the API now the intended direction for dmorph?
Also applies to: 57-59, 178-211
migration_test.go (5)
111-118: Nice: ctx-aware test migration implementation.Using tx.ExecContext with the passed ctx aligns the tests with the library’s new ctx-aware contract.
400-408: Great: WithLog option validated end-to-end.Good coverage that the logger flows via NewMorpher and is the instance used.
Also applies to: 404-412
426-434: Option validation coverage looks solid.The table name happy-path and failure cases (empty/invalid) are well covered and match the error contracts.
Also applies to: 439-446, 451-458
558-560: Nice negative-path coverage for internal wrappers (TapplyMigrations).These tests exercise register/commit error paths well.
Also applies to: 593-596, 647-650
290-291: Good direct exercise of the dialect EnsureMigrationTableExists with ctx.This ensures the dialect ctx-aware methods are reachable from tests.
…and precision - Standardized error descriptions by replacing "signalizes" with "signals" for grammatical correctness. - Refactored migration logic to encapsulate single migration execution in a new `runOneMigration` method. - Improved transaction handling with better error propagation and deferred rollback safety. - Simplified `WithMigrationsFromFS` function signature for better consistency. - Updated tests to utilize `require` over `assert` for stricter validation. - Enhanced `.golangci.yaml` configuration to ignore common variable names.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.golangci.yaml (3)
13-15: Fix invalid key: use linters.enable-all (not linters.default: all).golangci-lint doesn’t support “default: all”. Use “enable-all: true” and keep the disable list as-is.
Apply:
linters: - default: all + enable-all: true
24-42: Move non-existent “exclusions” block under issues.exclude-rules.The top-level “exclusions” and “warn-unused” keys are not recognized. Place path/linter rules under issues.exclude-rules. Keep your existing max-issues settings.
Apply:
- exclusions: - warn-unused: true - - rules: - - path: _test\.go - linters: - - cyclop - - dupl - - dupword - - err113 - - funlen - - gocognit - - maintidx - - nestif - - - path: examples/ - linters: - - err113And extend the issues block:
issues: max-issues-per-linter: 0 max-same-issues: 0 + exclude-rules: + # Don't run heavy complexity linters on non-test files + - path: ^(?!.*_test\.go).*$ # all files except *_test.go + linters: + - cyclop + - dupl + - dupword + - funlen + - gocognit + - maintidx + - nestif + # Scope err113 away from examples/ (or invert per your intent) + - path: ^examples/ + linters: + - err113Run to validate the config parses:
#!/usr/bin/env bash set -euo pipefail golangci-lint linters golangci-lint run -v --timeout=5mAlso applies to: 95-97
43-44: Rename “settings” to top-level “linters-settings”.Per schema, per-linter options live under linters-settings.
Apply:
- settings: +linters-settings:migration.go (1)
246-276: Transaction flow is solid; BeginTx with ctx and wrapped errors in place.The defer Rollback plus explicit rollbacks ensures resource cleanup and preserves rollback errors via errors.Join. LGTM.
🧹 Nitpick comments (8)
.golangci.yaml (1)
85-90: Fix YAML indentation under varnamelen.ignore-names.Current indentation triggers yamllint at Line 86. Indent list items two spaces deeper than the key.
Apply:
varnamelen: max-distance: 10 ignore-names: - - ctx - - db - - err - - tx + - ctx + - db + - err + - txfile_migration_test.go (1)
103-121: Use require for error assertions to fail fast (testifylint).These checks guard subsequent operations; prefer require.*.
Apply:
- tx, txErr := db.BeginTx(t.Context(), nil) - assert.NoError(t, txErr, "expected no tx error") + tx, txErr := db.BeginTx(t.Context(), nil) + require.NoError(t, txErr, "expected no tx error") @@ - err := dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) - assert.Error(t, err, "expected error") + err := dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) + require.Error(t, err, "expected error") @@ - tx, txErr = db.BeginTx(t.Context(), nil) - assert.NoError(t, txErr, "expected no tx error") + tx, txErr = db.BeginTx(t.Context(), nil) + require.NoError(t, txErr, "expected no tx error") @@ - err = dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) - assert.Error(t, err, "expected error") + err = dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) + require.Error(t, err, "expected error")file_migration.go (4)
129-144: Accept whitespace-only “;” separators.Compare a trimmed line to “;” so lines like " ; " also delimit steps. Harmless improvement, more robust in real-world SQL dumps.
Apply:
- if scanner.Text() == ";" { + if strings.TrimSpace(scanner.Text()) == ";" {
112-168: Wrap ExecContext errors with step and migration id for better diagnosability.Currently returns raw driver errors. Include step and migration id context; import fmt.
Apply:
import ( "bufio" "bytes" "context" "database/sql" + "fmt" "io" @@ - if _, err := tx.ExecContext(ctx, buf.String()); err != nil { - return err + if _, err := tx.ExecContext(ctx, buf.String()); err != nil { + return fmt.Errorf("apply migration %q step %d: %w", id, step, err) } @@ - if _, err := tx.ExecContext(ctx, buf.String()); err != nil { - return err + if _, err := tx.ExecContext(ctx, buf.String()); err != nil { + return fmt.Errorf("apply migration %q step %d (final): %w", id, step, err) }
112-160: Optional: rename parameter “id” to “migrationID” to satisfy varnamelen and improve clarity.Alternatively, add “id” to varnamelen.ignore-names in .golangci.yaml.
Apply:
-func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error { +func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, migrationID string, log *slog.Logger) error { @@ - log.Info("migration step", - slog.String("id", id), + log.Info("migration step", + slog.String("id", migrationID), slog.Int("step", step), ) @@ - return fmt.Errorf("apply migration %q step %d: %w", id, step, err) + return fmt.Errorf("apply migration %q step %d: %w", migrationID, step, err) } @@ - log.Info("migration step", - slog.String("id", id), + log.Info("migration step", + slog.String("id", migrationID), slog.Int("step", step), ) @@ - if _, err := tx.ExecContext(ctx, buf.String()); err != nil { - return fmt.Errorf("apply migration %q step %d (final): %w", id, step, err) + if _, err := tx.ExecContext(ctx, buf.String()); err != nil { + return fmt.Errorf("apply migration %q step %d (final): %w", migrationID, step, err) }
67-85: Optional: filter only “.sql” regular files in WithMigrationsFromFS to avoid accidental non-SQL ingestion.Safer default, still sorted later.
Apply:
- if entry.Type().IsRegular() { + if entry.Type().IsRegular() && strings.HasSuffix(entry.Name(), ".sql") { morpher.Migrations = append(morpher.Migrations, migrationFromFileFS(entry.Name(), d, morpher.Log)) }migration_test.go (2)
92-99: Use require for setup errors to fail fast.Setup failures invalidate the rest of the test; prefer require.NoError.
Apply:
-assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") +require.NoError(t, migrationsDirErr, "migrations directory could not be opened")Replicate at the other occurrences in these ranges.
Also applies to: 207-213, 244-251
484-491: Deduplicate test sentinel string (“utter nonsense”) into a constant.Reduces noise and satisfies goconst.
Apply:
@@ import ( @@ ) +const nonsenseSQL = "utter nonsense" + @@ - dialect := dmorph.DialectSQLite() - dialect.CreateTemplate = "utter nonsense" + dialect := dmorph.DialectSQLite() + dialect.CreateTemplate = nonsenseSQL @@ - dialect := dmorph.DialectSQLite() - dialect.AppliedTemplate = "utter nonsense" + dialect := dmorph.DialectSQLite() + dialect.AppliedTemplate = nonsenseSQL @@ - d.RegisterTemplate = "utter nonsense" + d.RegisterTemplate = nonsenseSQLAlso applies to: 515-521, 518-526
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.golangci.yaml(2 hunks)file_migration.go(7 hunks)file_migration_test.go(4 hunks)migration.go(10 hunks)migration_test.go(21 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:21:31.662Z
Learnt from: AlphaOne1
PR: AlphaOne1/dmorph#31
File: dialects.go:29-29
Timestamp: 2025-08-12T20:21:31.662Z
Learning: In the dmorph codebase, the maintainer AlphaOne1 prefers to avoid using context.Background() and context handling in database operations to reduce code complexity and clutter, even when suggested by linters like noctx.
Applied to files:
file_migration_test.gomigration.gomigration_test.go
🧬 Code graph analysis (3)
file_migration_test.go (4)
migration.go (1)
Run(311-319)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (1)
WithMigrationFromFile(36-55)exports_internal_test.go (2)
TmigrationFromFileFS(12-12)TapplyStepsStream(11-11)
file_migration.go (1)
migration.go (2)
MorphOption(82-82)Morpher(74-79)
migration_test.go (5)
migration.go (13)
Run(311-319)WithDialect(85-91)WithMigrations(94-100)Migration(56-59)Morpher(74-79)Dialect(49-53)ErrNoDialect(32-32)ErrNoMigrations(35-35)ErrNoMigrationTable(38-38)ErrMigrationTableNameInvalid(41-41)NewMorpher(133-150)WithLog(104-110)WithTableName(114-128)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(69-86)WithMigrationFromFileFS(59-65)FileMigration(19-23)WithMigrationFromFile(36-55)exports_internal_test.go (1)
TmigrationOrder(13-13)dialects.go (1)
BaseDialect(16-20)
🪛 golangci-lint (2.2.2)
file_migration_test.go
105-105: require-error: for error assertions use require
(testifylint)
109-109: require-error: for error assertions use require
(testifylint)
115-115: require-error: for error assertions use require
(testifylint)
file_migration.go
112-112: parameter name 'id' is too short for the scope of its usage
(varnamelen)
136-136: error returned from external package is unwrapped: sig: func (*database/sql.Tx).ExecContext(ctx context.Context, query string, args ...any) (database/sql.Result, error)
(wrapcheck)
migration_test.go
484-484: string utter nonsense has 3 occurrences, make it a constant
(goconst)
🪛 YAMLlint (1.37.1)
.golangci.yaml
[warning] 86-86: wrong indentation: expected 16 but found 14
(indentation)
🔇 Additional comments (5)
file_migration_test.go (1)
73-80: Good portability choice: DirFS.Switching to os.DirFS simplifies setup and avoids dependency on newer APIs. LGTM.
migration_test.go (2)
60-69: Good: context-aware Run and FS usage.Using context-aware API and fs.Sub aligns with the new public surface. LGTM.
612-620: Nice: switched to ExecContext with t.Context().Consistent with the context-aware migration flow.
migration.go (2)
228-235: Good guard: pre-transaction ctx cancellation check.Catches cancellations before starting work.
183-191: Enhanced error wrapping on dialect calls is appropriate.Wrapping EnsureMigrationTableExists and AppliedMigrations provides actionable context. LGTM.
Also applies to: 187-211
…ed error clarity - Replaced `assert` with `require` in tests for immediate validation. - Added file type check for `.sql` files in `file_migration.go`. - Enhanced error messages in `applyStepsStream` for better debugging. - Renamed `id` to `migrationID` for clarity. - Included `fmt.Errorf` for detailed error propagation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
migration_test.go (5)
46-58: Fail fast on setup errors: use require.NoError (don’t continue after fatal preconditions).Continuing after temp-file or DB open failures risks cascading errors and makes failures noisy. Require and abort early.
Apply this shape here (replicate in other tests):
- if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } + require.NoError(t, dbFileErr, "DB file could not be created") + defer func() { _ = os.Remove(dbFile) }() - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + require.NoError(t, dbErr, "DB file could not be created") + t.Cleanup(func() { _ = db.Close() })Replicate the same change in the other tests that have the same if/else pattern.
64-67: Prefer t.Context() over context.Background() in tests.Tie operations to the test lifecycle so they auto-cancel on failure/timeout and avoid stray background work. This also reduces explicit “background” noise. Noting your preference captured in learnings to avoid context.Background in DB operations, this aligns by using the test-provided context instead of constructing new ones.
Representative edits (apply the same pattern to the other occurrences in the listed ranges):
- runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir))- require.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(context.Background(), db, "migrations")) + require.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "migrations"))- assert.Error(t, - morpher.TapplyMigrations(context.Background(), db, ""), + assert.Error(t, + morpher.TapplyMigrations(t.Context(), db, ""), "morpher should fail to register")Also applies to: 94-105, 136-139, 147-147, 174-185, 211-221, 248-258, 286-286, 295-298, 459-459, 492-492, 524-524, 553-553, 589-589, 643-643
484-484: Deduplicate repeated literal “utter nonsense” (goconst).Lift the string to a const to appease the linter and improve clarity.
Add once near the imports:
const nonsenseSQL = "utter nonsense"Then update these lines:
- dialect.CreateTemplate = "utter nonsense" + dialect.CreateTemplate = nonsenseSQL- dialect.AppliedTemplate = "utter nonsense" + dialect.AppliedTemplate = nonsenseSQL- d.RegisterTemplate = "utter nonsense" + d.RegisterTemplate = nonsenseSQLAlso applies to: 516-516, 585-585
440-441: Fix minor grammar in assertion messages.These messages read oddly and one says the opposite of what the assertion checks.
- assert.Error(t, err, "morpher could created with empty table name") + assert.Error(t, err, "morpher could be created with empty table name")- assert.Error(t, err, "morpher could created with invalid table name") + assert.Error(t, err, "morpher could be created with invalid table name")- assert.Error(t, runErr, "morpher should run") + assert.Error(t, runErr, "morpher should not run")Also applies to: 452-453, 461-461
25-40: Make prepareDB a test helper and simplify with t.TempDir.This removes manual cleanup and error propagation boilerplate across tests.
Proposed helper (add once in this file):
func prepareDB(t *testing.T) string { t.Helper() dir := t.TempDir() return dir + "/test.db" }And a DRY DB open helper:
func openSQLite(t *testing.T, dsn string) *sql.DB { t.Helper() db, err := sql.Open("sqlite", dsn) require.NoError(t, err, "DB could not be opened") t.Cleanup(func() { _ = db.Close() }) return db }Then tests become:
dbFile := prepareDB(t) db := openSQLite(t, dbFile)If you want, I can push a follow-up commit applying this across the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
file_migration.go(7 hunks)file_migration_test.go(4 hunks)migration_test.go(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- file_migration_test.go
- file_migration.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:21:31.662Z
Learnt from: AlphaOne1
PR: AlphaOne1/dmorph#31
File: dialects.go:29-29
Timestamp: 2025-08-12T20:21:31.662Z
Learning: In the dmorph codebase, the maintainer AlphaOne1 prefers to avoid using context.Background() and context handling in database operations to reduce code complexity and clutter, even when suggested by linters like noctx.
Applied to files:
migration_test.go
🧬 Code graph analysis (1)
migration_test.go (5)
migration.go (18)
Run(311-319)WithDialect(85-91)WithMigrations(94-100)ErrMigrationsTooOld(46-46)ErrMigrationsUnrelated(25-25)ErrMigrationsUnsorted(29-29)Migration(56-59)Morpher(74-79)Dialect(49-53)ErrNoDialect(32-32)ErrNoMigrations(35-35)ErrNoMigrationTable(38-38)ErrMigrationTableNameInvalid(41-41)NewMorpher(133-150)WithLog(104-110)WithTableName(114-128)Run(283-291)m(173-206)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(70-87)WithMigrationFromFileFS(60-66)FileMigration(20-24)WithMigrationFromFile(37-56)exports_internal_test.go (1)
TmigrationOrder(13-13)dialects.go (1)
BaseDialect(16-20)
🪛 golangci-lint (2.2.2)
migration_test.go
484-484: string utter nonsense has 3 occurrences, make it a constant
(goconst)
🔇 Additional comments (1)
migration_test.go (1)
399-403: Nice: logger injection is exercised.Good coverage of WithLog and ensuring the logger is actually set on the Morpher.
- Replaced `context.Background()` with `t.Context()` in tests for better test isolation. - Standardized error messages in `assert` and `require` for improved clarity and consistency. - Minor grammatical corrections in error strings. - Removed unused `context` imports from test files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
dialects_test.go (2)
156-158: Use ExecContext with t.Context() for consistency with context-aware DB opsThis test already uses context-aware calls elsewhere; switch PRAGMA enabling to ExecContext as well.
- _, execErr := db.Exec("PRAGMA foreign_keys = ON") + _, execErr := db.ExecContext(t.Context(), "PRAGMA foreign_keys = ON")
122-161: Rename short var ‘d’ to ‘dialect’ to satisfy varnamelen and improve readabilityThe variable spans a long block and multiple uses; a descriptive name reads better and matches linter guidance.
-func TestEnsureMigrationTableExistsCommitError(t *testing.T) { - d := dmorph.BaseDialect{ +func TestEnsureMigrationTableExistsCommitError(t *testing.T) { + dialect := dmorph.BaseDialect{ @@ - assert.Error(t, d.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") + assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error")file_migration_test.go (2)
20-34: Prefer require + t.Cleanup to reduce boilerplate in setup/teardownSeveral tests manually if-else-defer the cleanup. Using require and t.Cleanup shortens code and fails fast. Consider applying this pattern across the file.
dbFile, dbFileErr := prepareDB() - -if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) -} else { - defer func() { _ = os.Remove(dbFile) }() -} +require.NoError(t, dbFileErr, "DB file could not be created") +t.Cleanup(func() { _ = os.Remove(dbFile) }) db, dbErr := sql.Open("sqlite", dbFile) - -if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) -} else { - defer func() { _ = db.Close() }() -} +require.NoError(t, dbErr, "DB file could not be created") +t.Cleanup(func() { _ = db.Close() })
73-79: Note: nil tx relies on file-open failure happening firstThis test passes a nil transaction to Migrate(). It currently works because the error originates from opening a non-existent file. If migrationFromFileFS changes order (e.g., touches tx before opening the file), this could panic. If you want future-proofing, consider creating a transient DB/tx and still target the non-existent file. Happy to draft that if desired.
migration_test.go (4)
22-26: Deduplicate repeated “utter nonsense” by introducing a file-level constantgolangci-lint flagged this string duplication; a small constant cleans it up and improves maintainability.
//go:embed testData var testMigrationsDir embed.FS + +const invalidSQL = "utter nonsense"
477-487: Use the new invalidSQL constant (avoids duplication)- dialect := dmorph.DialectSQLite() - dialect.CreateTemplate = "utter nonsense" + dialect := dmorph.DialectSQLite() + dialect.CreateTemplate = invalidSQL
509-521: Use the new invalidSQL constant here as well- dialect := dmorph.DialectSQLite() - dialect.AppliedTemplate = "utter nonsense" + dialect := dmorph.DialectSQLite() + dialect.AppliedTemplate = invalidSQL
68-82: Optional: standardize setup/teardown across tests to reduce repetitionMany tests still use if-else-defer patterns for file/DB handling. Consider a small helper like openTempSQLite(t) for consistency and brevity across the suite.
Example helper (place near prepareDB):
func openTempSQLite(t *testing.T) (*sql.DB, string) { t.Helper() dbFile, err := prepareDB() require.NoError(t, err, "DB file could not be created") t.Cleanup(func() { _ = os.Remove(dbFile) }) db, dbErr := sql.Open("sqlite", dbFile) require.NoError(t, dbErr, "DB could not be opened") t.Cleanup(func() { _ = db.Close() }) return db, dbFile }Then replace per-test boilerplate with:
db, _ := openTempSQLite(t)Also applies to: 114-128, 148-162, 185-199, 222-236, 260-274
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
dialects_test.go(6 hunks)file_migration_test.go(5 hunks)migration_test.go(20 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:21:31.662Z
Learnt from: AlphaOne1
PR: AlphaOne1/dmorph#31
File: dialects.go:29-29
Timestamp: 2025-08-12T20:21:31.662Z
Learning: In the dmorph codebase, the maintainer AlphaOne1 prefers to avoid using context.Background() and context handling in database operations to reduce code complexity and clutter, even when suggested by linters like noctx.
Applied to files:
migration_test.go
🧬 Code graph analysis (3)
dialects_test.go (3)
dialects.go (1)
BaseDialect(16-20)dialect_sqlite.go (1)
DialectSQLite(7-22)migration.go (2)
Dialect(73-78)EnsureMigrationTableExists(48-52)
file_migration_test.go (4)
migration.go (2)
Run(311-319)WithDialect(85-91)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (1)
WithMigrationFromFile(37-56)exports_internal_test.go (2)
TmigrationFromFileFS(12-12)TapplyStepsStream(11-11)
migration_test.go (5)
migration.go (8)
Run(311-319)WithDialect(85-91)WithMigrations(94-100)Migration(56-59)Morpher(74-79)Dialect(49-53)NewMorpher(133-150)m(173-206)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(70-87)WithMigrationFromFileFS(60-66)FileMigration(20-24)WithMigrationFromFile(37-56)exports_internal_test.go (1)
TmigrationOrder(13-13)dialects.go (1)
BaseDialect(16-20)
🪛 golangci-lint (2.2.2)
dialects_test.go
122-122: variable name 'd' is too short for the scope of its usage
(varnamelen)
migration_test.go
478-478: string utter nonsense has 3 occurrences, make it a constant
(goconst)
🔇 Additional comments (12)
dialects_test.go (1)
24-33: LGTM: tests exercise only the public dialect constructors from the external packageSwitching to dmorph_test and calling the public Dialect* constructors keeps tests aligned with the public API surface. This is the right direction for enforcing API boundaries.
file_migration_test.go (3)
36-40: LGTM: context-aware Run + WithMigrationFromFileGood use of t.Context() and the public option. This aligns with the rest of the suite’s context propagation.
61-68: LGTM: clear assertion for non-existent file via WithMigrationFromFileError path validation looks good; PathError type assertion is a nice touch.
103-121: LGTM: negative-path coverage for applyStepsStream with garbage inputGood use of BeginTx with t.Context(), require for preconditions, and explicit rollback. This hits an important error path.
migration_test.go (8)
46-53: LGTM: setup using require + t.Cleanup improves test hygieneThis style keeps setup concise and failures immediate.
58-62: LGTM: Run now correctly propagates t.Context()Consistent with the context-aware API introduced in the library.
106-109: LGTM: Migration impl uses ExecContext with ctxCorrectly adheres to the context-aware contract for migrations.
280-287: LGTM: deterministic prep for unordered applied migrationsEnsuring the table exists via the dialect and using ExecContext with t.Context() is solid.
541-550: LGTM: applyMigrations correctly errors on invalid/closed DBGood coverage for morpher.TapplyMigrations error path with a closed DB.
606-614: LGTM: context-aware ExecContext for PRAGMAMatches the suite’s context usage and satisfies noctx concerns.
570-585: Reasonable use of BaseDialect override for negative-path testingType assertion is validated with require.True; mutating RegisterTemplate to invalid SQL exercises the expected failure path.
141-144: Sanity check: Run(nil DB) error path is coveredGood to keep this guard; it protects accidental API misuse.
… and cleanup - Introduced `openTempSQLite` helper to simplify and standardize test database handling. - Replaced repetitive database setup and teardown logic with the new helper across all tests. - Removed redundant error checks and improved code readability in test files.
…losed database scenario
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
migration_test.go (1)
442-450: Good: assert type once before useThe safe type assertion for BaseDialect is appreciated; matches previous review guidance and prevents forcetypeassert panics.
🧹 Nitpick comments (9)
dialects_test.go (3)
10-13: Switch error assertions to require and check db.Close; fixes testifylint and errcheck findings
- Use require for error assertions so tests fail fast.
- Check db.Close return value to satisfy errcheck.
import ( "regexp" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/AlphaOne1/dmorph" )- db, _ := openTempSQLite(t) - db.Close() + db, _ := openTempSQLite(t) + require.NoError(t, db.Close()) - assert.Error(t, - dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "irrelevant"), - "expected error on closed database") + require.Error(t, + dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "irrelevant"), + "expected error on closed database") _, err := dmorph.DialectSQLite().AppliedMigrations(t.Context(), db, "irrelevant") - assert.Error(t, err, "expected error on closed database") + require.Error(t, err, "expected error on closed database")- assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") + require.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error")- _, execErr := db.ExecContext(t.Context(), "PRAGMA foreign_keys = ON") - assert.NoError(t, execErr, "foreign keys checking could not be enabled") + _, execErr := db.ExecContext(t.Context(), "PRAGMA foreign_keys = ON") + require.NoError(t, execErr, "foreign keys checking could not be enabled") - assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") + require.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error")Also applies to: 63-72, 85-88, 113-118
41-43: Nit: Contains + Regexp are redundant for “%s” placeholderEither check with Contains or Regexp; keeping both duplicates the same invariant and adds noise. I’d keep Contains for clarity.
- assert.Regexp(t, re, d.CreateTemplate) + // assert.Regexp(t, re, d.CreateTemplate) // redundant with Contains- assert.Regexp(t, re, d.AppliedTemplate) + // assert.Regexp(t, re, d.AppliedTemplate) // redundant with Contains- assert.Regexp(t, re, d.RegisterTemplate) + // assert.Regexp(t, re, d.RegisterTemplate) // redundant with ContainsAlso applies to: 48-50, 55-57
63-64: Follow-up if you adopt the openTempSQLite signature changeIf you refactor openTempSQLite to return only *sql.DB (see suggestion in migration_test.go), update this call site accordingly.
- db, _ := openTempSQLite(t) + db := openTempSQLite(t)file_migration_test.go (2)
77-79: Consistency: use require for the second error assertionThe earlier assertion in this test uses require. Make the second one require too to fail fast.
- assert.Error(t, err, "expected error") + require.Error(t, err, "expected error")
19-19: *If you simplify openTempSQLite to return only sql.DB, update call sitesSee migration_test.go suggestion. Here are the corresponding updates.
- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)Also applies to: 30-30, 55-55
migration_test.go (4)
42-54: Drop unused return from openTempSQLite; fix all call sites (unparam)The second return value (dbFile) isn’t used anywhere. Simplify the helper and update callers.
-func openTempSQLite(t *testing.T) (*sql.DB, string) { +func openTempSQLite(t *testing.T) *sql.DB { t.Helper() dbFile, err := prepareDB() require.NoError(t, err, "DB file could not be created") t.Cleanup(func() { _ = os.Remove(dbFile) }) db, dbErr := sql.Open("sqlite", dbFile) require.NoError(t, dbErr, "DB could not be opened") t.Cleanup(func() { _ = db.Close() }) - return db, dbFile + return db }- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)- db, _ := openTempSQLite(t) + db := openTempSQLite(t)Also applies to: 58-58, 74-74, 106-106, 126-126, 149-149, 172-172, 196-196, 383-383, 401-401, 419-419, 434-434, 456-456
6-17: Wrap external errors to satisfy wrapcheck; add fmt importWrap the ExecContext error with context. This helps diagnostics and clears wrapcheck.
import ( "context" "database/sql" "embed" + "fmt" "io/fs" "log/slog" "os" "testing"func (m TestMigrationImpl) Migrate(ctx context.Context, tx *sql.Tx) error { - _, err := tx.ExecContext(ctx, "CREATE TABLE t0 (id INTEGER PRIMARY KEY)") - return err + if _, err := tx.ExecContext(ctx, "CREATE TABLE t0 (id INTEGER PRIMARY KEY)"); err != nil { + return fmt.Errorf("create t0: %w", err) + } + return nil }Also applies to: 98-101
311-319: Deduplicate repeated “utter nonsense” test string (goconst)Use a shared constant for the repeated sentinel SQL to appease goconst and make intent clearer.
Add this constant near the top of the file (outside changed ranges):
const nonsenseSQL = "utter nonsense"Then update usages:
morpher, err := dmorph.NewMorpher( dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationFromFile("testData/01_base_table.sql"), + dmorph.WithMigrationFromFile("testData/01_base_table.sql"), dmorph.WithLog(l), )dialect := dmorph.DialectSQLite() - dialect.CreateTemplate = "utter nonsense" + dialect.CreateTemplate = nonsenseSQLdialect := dmorph.DialectSQLite() - dialect.AppliedTemplate = "utter nonsense" + dialect.AppliedTemplate = nonsenseSQLAlso applies to: 386-389, 403-409
202-203: Minor: you can inline the EnsureMigrationTableExists requireNot necessary, but consider inlining for brevity in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
dialects_test.go(4 hunks)file_migration_test.go(1 hunks)migration_test.go(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:21:31.662Z
Learnt from: AlphaOne1
PR: AlphaOne1/dmorph#31
File: dialects.go:29-29
Timestamp: 2025-08-12T20:21:31.662Z
Learning: In the dmorph codebase, the maintainer AlphaOne1 prefers to avoid using context.Background() and context handling in database operations to reduce code complexity and clutter, even when suggested by linters like noctx.
Applied to files:
migration_test.go
🧬 Code graph analysis (3)
migration_test.go (5)
migration.go (16)
Run(311-319)WithDialect(85-91)WithMigrations(94-100)ErrMigrationsTooOld(46-46)ErrMigrationsUnrelated(25-25)ErrMigrationsUnsorted(29-29)Migration(56-59)Morpher(74-79)Dialect(49-53)ErrNoDialect(32-32)ErrNoMigrations(35-35)ErrNoMigrationTable(38-38)ErrMigrationTableNameInvalid(41-41)NewMorpher(133-150)WithLog(104-110)WithTableName(114-128)dialect_sqlite.go (1)
DialectSQLite(7-22)file_migration.go (4)
WithMigrationsFromFS(70-87)WithMigrationFromFileFS(60-66)FileMigration(20-24)WithMigrationFromFile(37-56)exports_internal_test.go (1)
TmigrationOrder(13-13)dialects.go (1)
BaseDialect(16-20)
file_migration_test.go (3)
migration.go (2)
Run(311-319)Run(283-291)file_migration.go (1)
WithMigrationFromFile(37-56)exports_internal_test.go (2)
TmigrationFromFileFS(12-12)TapplyStepsStream(11-11)
dialects_test.go (9)
dialects.go (1)
BaseDialect(16-20)dialect_csvq.go (1)
DialectCSVQ(7-22)dialect_db2.go (1)
DialectDB2(7-31)dialect_mssql.go (1)
DialectMSSQL(7-27)dialect_mysql.go (1)
DialectMySQL(7-16)dialect_oracle.go (1)
DialectOracle(7-31)dialect_postgres.go (1)
DialectPostgres(7-22)dialect_sqlite.go (1)
DialectSQLite(7-22)migration.go (2)
Dialect(73-78)EnsureMigrationTableExists(48-52)
🪛 golangci-lint (2.2.2)
migration_test.go
42-42: openTempSQLite - result 1 (string) is never used
(unparam)
101-101: error returned from external package is unwrapped: sig: func (*database/sql.Tx).ExecContext(ctx context.Context, query string, args ...any) (database/sql.Result, error)
(wrapcheck)
386-386: string utter nonsense has 3 occurrences, make it a constant
(goconst)
dialects_test.go
64-64: Error return value of db.Close is not checked
(errcheck)
66-66: require-error: for error assertions use require
(testifylint)
115-115: require-error: for error assertions use require
(testifylint)
🔇 Additional comments (3)
file_migration_test.go (1)
44-47: Nice: using os.DirFS for test data improves portabilityGood switch to os.DirFS over os.OpenRoot(...).FS(); simpler and avoids unnecessary handle management.
migration_test.go (2)
202-210: Nice: context-aware Exec and explicit unordered setupGood use of ExecContext with t.Context() and a single call to seed unordered rows; this makes the failure deterministic.
464-466: Good: PRAGMA foreign_keys uses ExecContext with test contextThis aligns with the context-aware pattern across the suite.
…t cases accordingly
2 participants
Summary by CodeRabbit
Documentation
Tests
Public API
Refactor
Chores