Merged
Conversation
📝 WalkthroughWalkthroughThis PR renames Go packages from snake_case to camelCase conventions across the codebase: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sqlbuilder/builder.go (1)
341-342:⚠️ Potential issue | 🟠 MajorBug:
tagIsNullincorrectly setsat.IsNotNull = true.The case for
tagIsNullat line 341 setsat.IsNotNull = true, which is the opposite of the intended behavior. It should setat.IsNull = true.🐛 Proposed fix
case normTag == tagIsNull: - at.IsNotNull = true + at.IsNull = true case normTag == tagIsNotNull: at.IsNotNull = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqlbuilder/builder.go` around lines 341 - 342, The case handling the normalized tag "tagIsNull" incorrectly sets at.IsNotNull = true; update that branch so it sets at.IsNull = true instead (locate the switch/case that handles normTag == tagIsNull in sqlbuilder, change the assignment on the at struct accordingly), and run/update any unit tests that assert null vs not-null behavior for the related function/methods to ensure the fix is covered.
🧹 Nitpick comments (2)
sqlize.go (1)
254-269: Consider keeping a deprecatedArvoSchemawrapper for compatibility.Renaming this exported method is source-breaking. A wrapper can preserve backward compatibility while moving callers forward.
Proposed backward-compatible wrapper
+// Deprecated: use AvroSchema. +func (s Sqlize) ArvoSchema(needTables ...string) []string { + return s.AvroSchema(needTables...) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqlize.go` around lines 254 - 269, Add a backward-compatible wrapper named ArvoSchema that simply delegates to the existing AvroSchema method to avoid breaking callers: implement a new exported method ArvoSchema(needTables ...string) []string which calls and returns s.AvroSchema(needTables...), and mark it with a clear deprecation comment (e.g., "// Deprecated: ArvoSchema is deprecated; use AvroSchema instead.") so callers are guided to migrate while preserving compatibility.export/avro/builder.go (1)
12-13: Consider a deprecated compatibility shim for the renamed constructor.Renaming an exported function is source-breaking for external callers. A temporary wrapper keeps upgrade friction low.
Proposed backward-compatible shim
+// Deprecated: use NewAvroSchema. +func NewArvoSchema(table element.Table) *RecordSchema { + return NewAvroSchema(table) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@export/avro/builder.go` around lines 12 - 13, You renamed the exported constructor to NewAvroSchema which is source-breaking for callers; add a deprecated compatibility wrapper using the previous exported function name (the old constructor) that simply calls NewAvroSchema(table) and returns *RecordSchema, mark it with a deprecation comment, and update any internal call sites to use NewAvroSchema where appropriate so external users can migrate gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sqlize_test.go`:
- Line 991: The AvroSchema test assertion indexes got[0] and tt.want[0] without
verifying slice lengths, which can panic; update the condition around the
s.AvroSchema call to first ensure got and tt.want are non-nil and have length >
0 (e.g. check len(got) > 0 && len(tt.want) > 0) before calling
areEqualJSON(got[0], tt.want[0]), and otherwise assert nil/empty expectations
appropriately so the test never accesses index 0 on an empty slice.
---
Outside diff comments:
In `@sqlbuilder/builder.go`:
- Around line 341-342: The case handling the normalized tag "tagIsNull"
incorrectly sets at.IsNotNull = true; update that branch so it sets at.IsNull =
true instead (locate the switch/case that handles normTag == tagIsNull in
sqlbuilder, change the assignment on the at struct accordingly), and run/update
any unit tests that assert null vs not-null behavior for the related
function/methods to ensure the fix is covered.
---
Nitpick comments:
In `@export/avro/builder.go`:
- Around line 12-13: You renamed the exported constructor to NewAvroSchema which
is source-breaking for callers; add a deprecated compatibility wrapper using the
previous exported function name (the old constructor) that simply calls
NewAvroSchema(table) and returns *RecordSchema, mark it with a deprecation
comment, and update any internal call sites to use NewAvroSchema where
appropriate so external users can migrate gracefully.
In `@sqlize.go`:
- Around line 254-269: Add a backward-compatible wrapper named ArvoSchema that
simply delegates to the existing AvroSchema method to avoid breaking callers:
implement a new exported method ArvoSchema(needTables ...string) []string which
calls and returns s.AvroSchema(needTables...), and mark it with a clear
deprecation comment (e.g., "// Deprecated: ArvoSchema is deprecated; use
AvroSchema instead.") so callers are guided to migrate while preserving
compatibility.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.travis.ymlelement/column.goelement/migration.goexport/avro/builder.gooptions.gosqlbuilder/builder.gosqlbuilder/options.gosqlize.gosqlize_test.gosqlparser/mysql.gosqlparser/parser.gosqlparser/postgresql.gosqlparser/sqlite.gosqltemplates/ddl.gosqltemplates/option.gosqltemplates/type.go
| s.FromObjects(tt.args.models...) | ||
| if got := s.ArvoSchema(tt.args.needTables...); (got != nil || tt.want != nil) && !areEqualJSON(got[0], tt.want[0]) { | ||
| t.Errorf("ArvoSchema() mysql got = \n%v,\nexpected = \n%v", got, tt.want) | ||
| if got := s.AvroSchema(tt.args.needTables...); (got != nil || tt.want != nil) && !areEqualJSON(got[0], tt.want[0]) { |
There was a problem hiding this comment.
Guard slice access in the Avro assertion to avoid panic.
The current check can index got[0]/tt.want[0] without validating lengths first, which can panic and hide assertion intent.
Safer assertion pattern
- if got := s.AvroSchema(tt.args.needTables...); (got != nil || tt.want != nil) && !areEqualJSON(got[0], tt.want[0]) {
+ if got := s.AvroSchema(tt.args.needTables...); len(got) != len(tt.want) ||
+ (len(got) > 0 && !areEqualJSON(got[0], tt.want[0])) {
t.Errorf("AvroSchema() mysql got = \n%v,\nexpected = \n%v", got, tt.want)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sqlize_test.go` at line 991, The AvroSchema test assertion indexes got[0] and
tt.want[0] without verifying slice lengths, which can panic; update the
condition around the s.AvroSchema call to first ensure got and tt.want are
non-nil and have length > 0 (e.g. check len(got) > 0 && len(tt.want) > 0) before
calling areEqualJSON(got[0], tt.want[0]), and otherwise assert nil/empty
expectations appropriately so the test never accesses index 0 on an empty slice.
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.
Summary by CodeRabbit
Bug Fixes
Chores
Tests