Repository navigation
chore!: remove deprecated init/0 callback and atom metadata key - #35
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoves the deprecated init/0 callback from the event handler behaviour, exposes init/1/handle/2/handle_batch/1 in optional callbacks, simplifies after_start/1 to always return :ok, narrows assign_metadata/3 to binary keys, deletes init/0 tests and test helper modules, and adjusts test logging config/usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Supervisor
participant Subscription
participant Handler as EventHandler
rect rgb(250,240,245)
Note over Supervisor,Subscription: Old flow (with deprecated init/0)
Supervisor->>Subscription: start subscription
Subscription->>Handler: {:subscribed, _meta}
Handler->>Handler: (deprecated) call init/0
alt init/0 returns :ok
Handler->>Handler: call after_start(state) -> :ok
Handler->>Supervisor: continue
else init/0 returns {:stop, reason}
Handler->>Supervisor: stop(reason)
end
end
rect rgb(245,255,250)
Note over Supervisor,Subscription: New flow (init/0 removed)
Supervisor->>Subscription: start subscription
Subscription->>Handler: {:subscribed, _meta}
Handler->>Handler: call after_start(state) -> :ok
Handler->>Supervisor: continue (no init/0 path)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
8f80856 to
d7fbe24
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
d7fbe24 to
0b969c0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/commanded/middleware/pipeline.ex (1)
69-78:assign_metadata/3now intentionally enforces binary keysThe updated docstring and
when is_binary(key)guard line up with the goal of disallowing atom metadata keys. Existing middleware that passes atom keys will now fail with aFunctionClauseError, which is acceptable for a breaking change but worth calling out in upgrade notes. If you want a clearer failure mode, you could instead accept anykeyhere and raise anArgumentErrorwhen it’s not a binary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/commanded/event/handler.ex(2 hunks)lib/commanded/middleware/pipeline.ex(2 hunks)test/event/handler_init_test.exs(1 hunks)test/event/support/init/init_handler.ex(0 hunks)test/event_handler_after_start_test.exs(0 hunks)
💤 Files with no reviewable changes (2)
- test/event/support/init/init_handler.ex
- test/event_handler_after_start_test.exs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quality Assurance (1.18.x, 27)
🔇 Additional comments (3)
test/event/handler_init_test.exs (1)
5-5: Alias cleanup is consistent with remaining testsThe alias list now only includes modules actually referenced in this file (
EchoHandler,Handler,ReplyEvent,RuntimeConfigHandler), which matches the removal ofinit/0-specific tests and support modules.lib/commanded/event/handler.ex (2)
616-621:@optional_callbacksupdated to dropinit/0from the behaviourOnly
init/1is kept as an optional callback, which matches the behaviour definitions in this module and the removal of any@callback init/0. This cleanly severs the oldinit/0API surface while preserving the existing optional callbacks for error handling, partitioning, and batch/handle variants.
708-711: Defaultafter_start/1is now a no-op, no longer bridging toinit/0
after_start/1now simply returns:ok, so handlers that don’t override it will behave as before minus the legacyinit/0indirection. Custom handlers that rely onafter_start/1still get the same callback contract (:ok/{:ok, state}/{:stop, reason}); handlers that were depending oninit/0being invoked must now migrate toinit/1and/orafter_start/1, which is consistent with the breaking-change intent of this PR.
a261b91 to
d54240e
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
d54240e to
228399e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/event/handler_init_test.exs (1)
5-5: Orphaned test support file.The file
test/event/support/init/init_handler.exdefiningInitHandlerwith the deprecatedinit/0callback should be deleted as it's no longer referenced.
🧹 Nitpick comments (1)
test/example_domain/bank_account/bank_account_batch_handler.ex (1)
27-28: Consider removing the empty batch clause.The empty batch handler at line 28 appears unnecessary since
handle_batch/1at line 31 recursively processes the list and naturally terminates when the list is empty. The explicit empty clause adds no additional behavior.Apply this diff to remove the redundant clause:
- @impl Handler - def handle_batch([]), do: :ok - @impl Handler def handle_batch([{%BankAccountOpened{} = event, _metadata} | rest]) do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
config/test.exs(1 hunks)lib/commanded/event/handler.ex(2 hunks)lib/commanded/middleware/pipeline.ex(2 hunks)test/aggregates/aggregate_test.exs(1 hunks)test/event/handler_init_test.exs(1 hunks)test/event/support/init/init_handler.ex(0 hunks)test/event_handler_after_start_test.exs(0 hunks)test/example_domain/bank_account/bank_account_batch_handler.ex(1 hunks)
💤 Files with no reviewable changes (2)
- test/event_handler_after_start_test.exs
- test/event/support/init/init_handler.ex
🧰 Additional context used
🧬 Code graph analysis (1)
test/example_domain/bank_account/bank_account_batch_handler.ex (2)
lib/commanded/event/handler.ex (4)
after_start(709-711)start_link(656-662)start_link(830-854)handle_batch(1126-1128)test/event_handler_after_start_test.exs (1)
after_start(23-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quality Assurance (1.18.x, 27)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (11)
lib/commanded/middleware/pipeline.ex (1)
72-74: Verified: Breaking change is safe; no atom key usage found in codebase.The removal of atom key support has been verified. The only existing call to
assign_metadatain the codebase (in tests) already uses a binary key ("updated_by"), confirming no callers will break from the narrowed guard clause. The implementation is correct.config/test.exs (1)
6-7: LGTM!Raising the global logger level to
:inforeduces log noise during tests while still allowing individual tests to capture debug logs when needed (via ExUnit'scapture_logoption at line 11).test/aggregates/aggregate_test.exs (1)
41-47: LGTM!Correctly coordinates with the global logger level change. The
Logger.configure(level: :debug)ensures debug logs are emitted, andcapture_log([level: :debug])captures them for assertion.test/event/handler_init_test.exs (1)
8-44: LGTM!The test suite correctly focuses on
init/1callback functionality and runtime configuration, with allinit/0related tests properly removed.lib/commanded/event/handler.ex (3)
616-621: LGTM!Correctly updates the optional callbacks to reflect the API changes: removes
init: 0, retainsinit: 1, and addshandle: 2andhandle_batch: 1which have default implementations generated by__before_compile__.
709-710: LGTM!Default
after_start/1simplified to a no-op, removing the deprecatedinit/0fallback path. This aligns with the PR's breaking changes.
915-936: LGTM!The subscription handler correctly invokes only
after_start/1, with proper handling of all valid return values. The deprecatedinit/0path and deprecation warning have been cleanly removed.test/example_domain/bank_account/bank_account_batch_handler.ex (4)
10-10: LGTM!Alias added to support the
@implannotations introduced throughout the file.
13-20: LGTM!Correctly migrates from
init/0toafter_start/1and uses the proper return value{:stop, :unable_to_start}on error. This addresses the past review comment about the invalid return value.
22-25: LGTM!Proper
@implannotation added to clarify this implements theHandlerbehaviour callback.
30-39: LGTM!Proper
@implannotation and correct batch processing logic with tail-recursive pattern.
Note
Remove deprecated
init/0callback across handler and tests; enforce binary metadata keys in Pipeline; update example handler and test configurations.init/0inCommanded.Event.Handler(callback, invocation, warnings, and optional callbacks).after_start/1as the initialization path; subscribed flow now calls onlyafter_start/1.Commanded.Middleware.Pipeline.assign_metadata/3(docs and guard updated).init/0withafter_start/1inBankAccountBatchHandler; return{:stop, reason}on failure; add@implannotations.init/0.after_start/1; tweak log capture levels where needed.:info(from:debug).Written by Cursor Bugbot for commit 228399e. This will update automatically on new commits. Configure here.