refactor(rain): split query builders and tests into focused files#25
Merged
Conversation
Contributor
Greptile SummaryThis PR refactors the 1 491-line monolithic Key changes:
Confidence Score: 5/5Safe to merge — purely structural refactoring with no behavioral or API changes. All production logic is a verbatim file-level split from the original query.go; no algorithmic changes, no public API changes, and the test suite (including integration and benchmark tests) is confirmed green. The only findings are a duplicate test assertion and missing t.Parallel() calls — both P2 style issues that do not affect correctness or reliability. query_compile_internal_test.go (duplicate ErrNoConnection assertion); query_insert_test.go and query_write_test.go (missing t.Parallel() on top-level tests). Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+43
to
+50
| if err := (&InsertQuery{dialect: db.Dialect(), table: users.TableDef(), returning: []schema.Expression{users.ID}}).Scan(context.Background(), &internalUserRow{}); !errors.Is(err, ErrNoConnection) { | ||
| t.Fatalf("expected insert scan ErrNoConnection, got %v", err) | ||
| } | ||
|
|
||
| insertNoRunner := &InsertQuery{dialect: db.Dialect(), table: users.TableDef(), returning: []schema.Expression{users.ID}} | ||
| if err := insertNoRunner.Scan(context.Background(), &internalUserRow{}); !errors.Is(err, ErrNoConnection) { | ||
| t.Fatalf("expected insert returning scan ErrNoConnection, got %v", err) | ||
| } |
Contributor
There was a problem hiding this comment.
ErrNoConnection assertion for InsertQuery.Scan
Lines 43–45 and 47–50 create an identical InsertQuery (no runner, same table and returning) and both assert that Scan returns ErrNoConnection. The second block is redundant and should be removed.
Suggested change
| if err := (&InsertQuery{dialect: db.Dialect(), table: users.TableDef(), returning: []schema.Expression{users.ID}}).Scan(context.Background(), &internalUserRow{}); !errors.Is(err, ErrNoConnection) { | |
| t.Fatalf("expected insert scan ErrNoConnection, got %v", err) | |
| } | |
| insertNoRunner := &InsertQuery{dialect: db.Dialect(), table: users.TableDef(), returning: []schema.Expression{users.ID}} | |
| if err := insertNoRunner.Scan(context.Background(), &internalUserRow{}); !errors.Is(err, ErrNoConnection) { | |
| t.Fatalf("expected insert returning scan ErrNoConnection, got %v", err) | |
| } | |
| insertNoRunner := &InsertQuery{dialect: db.Dialect(), table: users.TableDef(), returning: []schema.Expression{users.ID}} | |
| if err := insertNoRunner.Scan(context.Background(), &internalUserRow{}); !errors.Is(err, ErrNoConnection) { | |
| t.Fatalf("expected insert scan ErrNoConnection, got %v", err) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/query_compile_internal_test.go
Line: 43-50
Comment:
**Duplicate `ErrNoConnection` assertion for `InsertQuery.Scan`**
Lines 43–45 and 47–50 create an identical `InsertQuery` (no runner, same `table` and `returning`) and both assert that `Scan` returns `ErrNoConnection`. The second block is redundant and should be removed.
```suggestion
insertNoRunner := &InsertQuery{dialect: db.Dialect(), table: users.TableDef(), returning: []schema.Expression{users.ID}}
if err := insertNoRunner.Scan(context.Background(), &internalUserRow{}); !errors.Is(err, ErrNoConnection) {
t.Fatalf("expected insert scan ErrNoConnection, got %v", err)
}
```
How can I resolve this? If you propose a fix, please make it concise.| "github.com/hyperlocalise/rain-orm/pkg/schema" | ||
| ) | ||
|
|
||
| func TestSelectToSQL(t *testing.T) { |
Contributor
There was a problem hiding this comment.
t.Parallel() on top-level test functions
TestSelectToSQL (this file, line 13) is missing t.Parallel(). The same pattern applies to several top-level functions in the other new test files: all eight TestInsert* functions in query_insert_test.go, and TestInsertUpdateDeleteToSQL, TestReturningUnsupportedDialect, and TestReturningSupportedOperations in query_write_test.go.
All of these are pure SQL-compilation tests with no shared mutable state, so they are safe to run in parallel. Adding t.Parallel() aligns them with the rest of the suite (e.g. TestExpandedTypesCompileToSQL, TestSelectAdvancedComposition, TestDialectFeatures) and avoids unnecessarily serialising an otherwise parallel test run.
Suggested change
| func TestSelectToSQL(t *testing.T) { | |
| func TestSelectToSQL(t *testing.T) { | |
| t.Parallel() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/query_select_test.go
Line: 13
Comment:
**Missing `t.Parallel()` on top-level test functions**
`TestSelectToSQL` (this file, line 13) is missing `t.Parallel()`. The same pattern applies to several top-level functions in the other new test files: all eight `TestInsert*` functions in `query_insert_test.go`, and `TestInsertUpdateDeleteToSQL`, `TestReturningUnsupportedDialect`, and `TestReturningSupportedOperations` in `query_write_test.go`.
All of these are pure SQL-compilation tests with no shared mutable state, so they are safe to run in parallel. Adding `t.Parallel()` aligns them with the rest of the suite (e.g. `TestExpandedTypesCompileToSQL`, `TestSelectAdvancedComposition`, `TestDialectFeatures`) and avoids unnecessarily serialising an otherwise parallel test run.
```suggestion
func TestSelectToSQL(t *testing.T) {
t.Parallel()
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR for?
Refactors the monolithic query builder implementation into focused file-level units for select, insert, update, delete, SQL compilation, and shared helpers, without changing behavior or package boundaries. Also reorganizes the query test suite into matching focused test files while preserving existing assertions and keeping the full fmt, lint, and test suite green.
What did you do for validating the changes?
Anything else you want to add? (Optional)