Conversation
Enable tagliatelle linter to enforce Effective Go naming conventions on struct tags (json, yaml, xml, bson, env). Exclude generated SDK code in pkg/client from linting. Add nolint directives for targetId fields to preserve JSON API backward compatibility.
…positives Exclude pkg/client from golangci-lint analysis as it contains generated SDK code. Add nolint:gocyclo directives on three functions where high cyclomatic complexity is structural (string mapping, AST visitor, bytecode interpreter) rather than indicative of actual logic complexity.
Remove unnecessary type conversions for RuntimeType and string casts. Add nolint:unparam directives where parameters are intentionally kept for API consistency, recursive propagation, or test flexibility.
Handle the error return from json.Marshal instead of silently discarding it. Panic on failure as DocID contains only basic types that cannot fail to marshal.
Add t.Parallel() to test functions and subtests where it was missing, ensuring both parent tests and their subtests call t.Parallel() consistently across API v1, API v2, VM, replication, storage, and accounts test files.
Enable usetesting linter and replace context.Background() and context.TODO() with t.Context() (or b.Context()) in test files. This ensures test contexts are properly cancelled when tests finish, preventing resource leaks.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded many test parallelizations and test-context migrations; sprinkled lint-suppression annotations; minor runtime changes: new controller event-handling path, parser selection tweak, and stricter JSON error handling in an Elasticsearch driver. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/ledger/controller_with_events.go (1)
30-42:⚠️ Potential issue | 🟠 MajorDefer events when a parent transaction is active.
When
c.hasTxis false butc.parent.hasTxis true (e.g., a controller returned byLockLedgerwithin a transaction),handleEventexecutes immediately and bypasses commit-time dispatch. This breaks the “emit at commit” semantics for nested controllers.🔧 Suggested fix (prioritize parent tx before immediate execution)
func (c *ControllerWithEvents) handleEvent(ctx context.Context, fn func()) { - if !c.hasTx { - fn() - return - } - if c.parent != nil && c.parent.hasTx { + if c.parent != nil && c.parent.hasTx { c.parent.handleEvent(ctx, fn) return } + if !c.hasTx { + fn() + return + } c.atCommit = append(c.atCommit, fn) }
🧹 Nitpick comments (1)
internal/storage/ledger/accounts_test.go (1)
327-329: Consider usingctxfor consistency within this test.The test declares
ctx := logging.TestingContext()at line 321 and uses it forUpdateAccountsMetadataat line 323, but this call now usest.Context(). This creates an inconsistency where one operation has the logging context and the other doesn't.For consistency with both this test function and all other tests in this file, consider using
ctxhere.Suggested change
- account, err := store.Accounts().GetOne(t.Context(), common.ResourceQuery[any]{ + account, err := store.Accounts().GetOne(ctx, common.ResourceQuery[any]{
…ontext canceled error t.Context() is canceled before t.Cleanup runs, causing batcher.Stop() to fail immediately instead of waiting for graceful shutdown.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1258 +/- ##
==========================================
- Coverage 82.20% 78.07% -4.14%
==========================================
Files 198 205 +7
Lines 10224 10979 +755
==========================================
+ Hits 8405 8572 +167
- Misses 1326 1555 +229
- Partials 493 852 +359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
goCamelconvention following Effective Go naming rules (json, yaml, xml, bson, env tags)pkg/client(generated SDK code) from lintingjson.Marshalerrorcontext.Background()/context.TODO()witht.Context()in testsTest plan
just lintpasses with 0 issuesgo test -race ./...passes all unit tests