Reorganize Imperium into per-bounded-context namespaces with qualified facades#136
Merged
Merged
Conversation
Reorganize src/Imperium flat layout into per-context folders (Primitives, Contract, Gameplay, Accounting, Rondel) without namespace or module changes. Imperium.fsproj compile order is updated to match the new paths. Preparation step for migrating each bounded context onto its own namespace + qualified facade module per the recommended approach in /tmp/research-topic.md.
- Replace 'namespace Imperium; module Accounting' with 'namespace Imperium.Accounting' across the bounded context - Split Accounting.fsi/.fs into Types.fsi/.fs (commands, events, dependencies, contract transformations) and Handlers.fs (internal command handlers), keeping the type+companion-module pairs together in the same file (FS0250 disallows splitting them across files within an assembly) - Add a [<RequireQualifiedAccess>] module Accounting facade exposing only val execute as the public router - Update callers (Imperium.Terminal.Accounting.Host and the Accounting/RondelHost test modules) to call Accounting.execute and to open Imperium.Accounting rather than abbreviating the former module First bounded context migration toward the recommended layout in /tmp/research-topic.md. Rondel and Gameplay follow.
- Replace 'namespace Imperium; module Rondel' with
'namespace Imperium.Rondel' across the bounded context
- Split the 816-line Rondel.fs/.fsi into per-concept files:
Types value types, commands, events, outbound/inbound
routing DUs, query types/views, and the
type-companion transformation modules that must
live next to their types (FS0250)
State RondelState, PendingMovement, their companions,
plus dependency types (LoadRondelState,
RondelEffects, CommitRondelEffects, etc.)
Movement internal pure Move decision pipeline (renamed
from internal module Move to module Movement to
align with the recommended layout)
Invoices internal pure OnInvoicePaid / OnInvoicePaymentFailed
Handlers internal load -> pure execute -> effects glue
plus the SetToStartingPositions pure module and
a private toEffects tuple lifter
Queries internal getNationPositions / getRondelOverview
- Add a [<RequireQualifiedAccess>] module Rondel facade exposing
only val execute, handle, getNationPositions, getRondelOverview
- Expose cross-file helper functions (Space.toString, fromString,
distance and RondelBillingId.create, newId) as val internal in
the .fsi files; the .fsi controls assembly-internal visibility
once a signature exists for a file
- Update callers (Imperium.Terminal.Rondel.Host and the Rondel
test module) to call Rondel.execute / Rondel.handle /
Rondel.getNationPositions / Rondel.getRondelOverview
Second bounded context migration toward the recommended layout
in /tmp/research-topic.md. Gameplay follows.
Replace 'namespace Imperium; module Gameplay' with 'namespace Imperium.Gameplay' and wrap the placeholder module in [<RequireQualifiedAccess>] module Gameplay. The placeholder, the internal GameId struct, and the internal NationId DU remain inside the facade module since they are not yet exposed. GameplayTests already opened Imperium.Gameplay; the change from module-open to namespace-open is transparent because the test file references no Gameplay members yet. Third (and final) bounded context migration toward the recommended layout in /tmp/research-topic.md.
Update the Project Structure and Rondel Implementation Patterns sections to describe: - The folder-per-bounded-context layout under src/Imperium/ (Primitives, Contract, Gameplay, Accounting, Rondel) with the per-context compile order - The namespace + facade convention: each BC lives in namespace Imperium.<BC> and exposes routers/queries only via a same-named [<RequireQualifiedAccess>] module facade, called as 'open Imperium.Rondel; Rondel.execute deps cmd' - The FS0250 constraint that forces type+companion-module pairs to live in the same .fsi/.fs pair (folded into Types/State rather than a standalone Transformations file) - The .fsi visibility rule: anything not in the signature file is private to its implementation file once a .fsi exists, so cross-file helpers (Space.toString/fromString/distance, RondelBillingId.create/newId) are exposed as val internal - The Rondel file split into Types, State, Movement, Invoices, Handlers, Queries, Rondel and the rename of the pure module Move -> Movement - Per-file locations for state transformations and handler glue (RondelState in State.fs, Movement in Movement.fs, etc.)
Both Accounting and Rondel previously co-located their dependency type aliases (handler input/output contracts) with the rest of the domain types or with state. Lift them into a sibling Dependencies.fsi/.fs pair per bounded context so each file owns one concern: - Accounting/Dependencies.fsi/.fs holds PublishAccountingEvent and AccountingDependencies (compiled after Accounting/Types) - Rondel/Dependencies.fsi/.fs holds LoadRondelState, RondelDependencies, RondelEffects, CommitRondelEffects, LoadRondelStateForQuery, RondelQueryDependencies (compiled after Rondel/State) State.fsi/.fs and Types.fsi/.fs shrink accordingly; no behaviour or public-API change.
Drop the FS0250 error code reference from AGENTS.md and state the rule directly: a module that shares a type's name must live in the same .fsi/.fs pair as the type. The compiler diagnostic was incidental implementation trivia; the architectural rule is what future contributors actually need.
Each bounded context now isolates its message types into their
own files instead of grouping them under Types:
Rondel/
Commands.fsi/.fs RondelCommand + SetToStartingPositionsCommand
+ MoveCommand with their fromContract modules,
plus the outbound RondelOutboundCommand +
ChargeMovement/VoidCharge variants with their
toContract modules
Events.fsi/.fs RondelEvent + PositionedAtStart/ActionDetermined/
MoveToActionSpaceRejected with RondelEvent.toContract,
plus the inbound RondelInboundEvent +
InvoicePaid/InvoicePaymentFailed with their
fromContract modules
Types.fsi/.fs trimmed to value types (RondelBillingId, Action,
Space) and query types/views only
Accounting/
Commands.fsi/.fs AccountingCommand + ChargeNationForRondelMovement
+ VoidRondelCharge with their fromContract modules
Events.fsi/.fs AccountingEvent + RondelInvoicePaid/PaymentFailed
with AccountingEvent.toContract
Types.fsi/.fs deleted (no value types left after the move)
Compile order: Types -> Commands -> Events -> State -> Dependencies
-> Movement/Invoices/Handlers/Queries -> Rondel for Rondel;
Commands -> Events -> Dependencies -> Handlers -> Accounting for
Accounting. Action.toString is now exposed as val internal in
Rondel/Types.fsi so RondelEvent.toContract can reach it from
Events.fs.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR restructures the Accounting and Rondel bounded contexts from monolithic top-level modules into organized folder hierarchies with separated concerns: domain command/event contracts, dependencies, internal handlers, and public facades using qualified access and effect-based routing. The project file is updated accordingly, and all hosts and tests are requalified to use the new module names. ChangesBounded-Context Modularization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/Imperium/Accounting/Events.fs (1)
6-22: ⚡ Quick winAdd standard section dividers and XML docs in the implementation file.
The
.fsiis structured/documented, but this.fsfile currently omits that pattern; keeping both aligned improves maintainability.Suggested direction
+// ────────────────────────────────────────────────────────────────────────── +// Events +// ────────────────────────────────────────────────────────────────────────── + +/// Integration events published by Accounting to notify other bounded contexts. type AccountingEvent = | RondelInvoicePaid of RondelInvoicePaidEvent | RondelInvoicePaymentFailed of RondelInvoicePaymentFailedEvent ... +/// Transform Domain event to Contract event for cross-boundary communication. module AccountingEvent =As per coding guidelines:
**/{Rondel,Accounting}/**/*.{fs,fsi}domain modules should follow a consistent sectioned structure with visual dividers and XML doc comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Imperium/Accounting/Events.fs` around lines 6 - 22, Add the same section dividers and XML documentation present in the .fsi to this implementation: annotate the AccountingEvent union and the related types (RondelInvoicePaidEvent, RondelInvoicePaymentFailedEvent) with XML doc comments and split the module into the standard visual sections (e.g., header, types, module functions) so the implementation mirrors the .fsi structure; ensure the toContract function has a brief XML doc and is placed under the expected "implementation" or "converters" divider to match coding guidelines.src/Imperium/Accounting/Commands.fs (1)
7-33: ⚡ Quick winAlign this
.fsfile with the domain section/doc structure used in.fsi.Please add the standard visual section dividers and XML docs in the implementation file as well, so the
.fs/.fsipair stays consistent and easier to scan.Suggested direction
+// ────────────────────────────────────────────────────────────────────────── +// Commands +// ────────────────────────────────────────────────────────────────────────── + +/// Union of all accounting commands for routing and dispatch. type AccountingCommand = | ChargeNationForRondelMovement of ChargeNationForRondelMovementCommand | VoidRondelCharge of VoidRondelChargeCommand ... +// ────────────────────────────────────────────────────────────────────────── +// Transformations +// ──────────────────────────────────────────────────────────────────────────As per coding guidelines:
**/{Rondel,Accounting}/**/*.{fs,fsi}domain modules should follow a consistent sectioned structure with visual dividers and XML doc comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Imperium/Accounting/Commands.fs` around lines 7 - 33, Add the same visual section dividers and XML documentation comments to this implementation file to match the .fsi structure: annotate the top-level type AccountingCommand and the record types ChargeNationForRondelMovementCommand and VoidRondelChargeCommand with the corresponding XML docs, and add the same commented visual divider blocks around the "Modules" area and each module implementation (ChargeNationForRondelMovementCommand and VoidRondelChargeCommand) so the .fs mirrors the .fsi domain sectioning and is easier to scan.src/Imperium/Rondel/State.fs (1)
93-118: ⚡ Quick winRefactor mutable accumulators in
src/Imperium/Rondel/State.fs
ThenationPositionsandpendingMovementscomputations uselet mutablewith in-loop updates; rewriting them asSeq.foldkeeps the code expression-based and consistent with the project style.♻️ Suggested refactor
- let nationPositions = - result { - let mutable positions = Map.empty - - for nation, position in state.NationPositions |> Map.toSeq do - match position with - | None -> positions <- positions |> Map.add nation None - | Some value -> - let! space = Space.fromString value - positions <- positions |> Map.add nation (Some space) - - return positions - } + let nationPositions = + state.NationPositions + |> Map.toSeq + |> Seq.fold + (fun current (nation, position) -> + result { + let! positions = current + match position with + | None -> return positions |> Map.add nation None + | Some value -> + let! space = Space.fromString value + return positions |> Map.add nation (Some space) + }) + (Ok Map.empty) - let pendingMovements = - result { - let mutable movements = Map.empty - - for nation, pending in state.PendingMovements |> Map.toSeq do - if pending.Nation <> nation then - return! Error $"Pending movement nation mismatch for {nation}." - else - let! mapped = PendingMovement.fromContract pending - movements <- movements |> Map.add nation mapped - - return movements - } + let pendingMovements = + state.PendingMovements + |> Map.toSeq + |> Seq.fold + (fun current (nation, pending) -> + result { + let! movements = current + if pending.Nation <> nation then + return! Error $"Pending movement nation mismatch for {nation}." + else + let! mapped = PendingMovement.fromContract pending + return movements |> Map.add nation mapped + }) + (Ok Map.empty)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Imperium/Rondel/State.fs` around lines 93 - 118, Replace the mutable for-loops in the result computations for nationPositions and pendingMovements with expression-based Seq.fold operations: for nationPositions fold over state.NationPositions |> Map.toSeq, calling Space.fromString for Some values and accumulating into a Map without mutability; for pendingMovements fold over state.PendingMovements |> Map.toSeq, validate pending.Nation equals nation (propagate Error with return! when mismatch) and call PendingMovement.fromContract to map and add entries to the accumulator Map; ensure you preserve the result computation context and error propagation semantics used by Space.fromString and PendingMovement.fromContract.src/Imperium/Rondel/Movement.fs (1)
8-14: ⚡ Quick winMark
MoveOutcomeexplicitly asinternal.Please declare this as
type internal MoveOutcometo align with the handler-internal type visibility rule.🔧 Suggested change
- type MoveOutcome = + type internal MoveOutcome =As per coding guidelines:
**/*.fs: Mark handler-internal types withtype internalin.fsfiles (e.g.,type internal MoveOutcome = ...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Imperium/Rondel/Movement.fs` around lines 8 - 14, Change the MoveOutcome discriminated union to be internal by updating its declaration to "type internal MoveOutcome = ..."; locate the MoveOutcome type in Movement.fs and prepend the internal keyword so the union (constructors Rejected, Free, FreeWithSupersedingUnpaidMovement, Paid, PaidWithSupersedingUnpaidMovement) is handler-internal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 98: Update the stale file-layout sentence so it no longer references
"Accounting/Types.fsi/.fs" and instead describes the PR’s current module split
into Commands, Events, Dependencies, Handlers and Accounting files; keep the
rest of the sentence about the stateless skeleton and the behavior of
Handlers.chargeNationForRondelMovement (auto-approves and publishes
RondelInvoicePaid) and Handlers.voidRondelCharge intact so readers understand
the implementation and future extension points.
In `@src/Imperium/Accounting/Accounting.fsi`:
- Around line 7-9: Update the XML doc comment for the public function execute to
describe each parameter and the return semantics: document the
AccountingDependencies parameter (what dependencies are required/used), document
the AccountingCommand parameter (what kinds of commands are accepted and any
preconditions), and describe the Async<unit> return contract (what success
represents, whether exceptions are raised on failure, and any cancellation
behavior via the implicit CancellationToken). Keep the triple-slash XML style
and include <param name="..."> tags for AccountingDependencies and
AccountingCommand and a <returns> tag for the Async<unit> outcome so the .fsi
exposes full parameter and return semantics for the execute signature.
In `@src/Imperium/Rondel/Handlers.fs`:
- Around line 25-50: The four handlers (setToStartingPositions, move,
onInvoicePaid, onInvoicePaymentFailed) currently accept a LoadRondelState
parameter; change them to accept a single RondelDependencies parameter instead
and use its loader to fetch state before calling the existing logic. Concretely,
update the function signatures to take (deps: RondelDependencies), obtain the
loader from deps (e.g. deps.Load or deps.load depending on the record field name
used in your codebase), call that loader with command.GameId/event.GameId as
before, then pass the loaded state into SetToStartingPositions.execute,
Movement.execute, OnInvoicePaid.handle, and OnInvoicePaymentFailed.handle and
pipe the results to toEffects unchanged.
---
Nitpick comments:
In `@src/Imperium/Accounting/Commands.fs`:
- Around line 7-33: Add the same visual section dividers and XML documentation
comments to this implementation file to match the .fsi structure: annotate the
top-level type AccountingCommand and the record types
ChargeNationForRondelMovementCommand and VoidRondelChargeCommand with the
corresponding XML docs, and add the same commented visual divider blocks around
the "Modules" area and each module implementation
(ChargeNationForRondelMovementCommand and VoidRondelChargeCommand) so the .fs
mirrors the .fsi domain sectioning and is easier to scan.
In `@src/Imperium/Accounting/Events.fs`:
- Around line 6-22: Add the same section dividers and XML documentation present
in the .fsi to this implementation: annotate the AccountingEvent union and the
related types (RondelInvoicePaidEvent, RondelInvoicePaymentFailedEvent) with XML
doc comments and split the module into the standard visual sections (e.g.,
header, types, module functions) so the implementation mirrors the .fsi
structure; ensure the toContract function has a brief XML doc and is placed
under the expected "implementation" or "converters" divider to match coding
guidelines.
In `@src/Imperium/Rondel/Movement.fs`:
- Around line 8-14: Change the MoveOutcome discriminated union to be internal by
updating its declaration to "type internal MoveOutcome = ..."; locate the
MoveOutcome type in Movement.fs and prepend the internal keyword so the union
(constructors Rejected, Free, FreeWithSupersedingUnpaidMovement, Paid,
PaidWithSupersedingUnpaidMovement) is handler-internal.
In `@src/Imperium/Rondel/State.fs`:
- Around line 93-118: Replace the mutable for-loops in the result computations
for nationPositions and pendingMovements with expression-based Seq.fold
operations: for nationPositions fold over state.NationPositions |> Map.toSeq,
calling Space.fromString for Some values and accumulating into a Map without
mutability; for pendingMovements fold over state.PendingMovements |> Map.toSeq,
validate pending.Nation equals nation (propagate Error with return! when
mismatch) and call PendingMovement.fromContract to map and add entries to the
accumulator Map; ensure you preserve the result computation context and error
propagation semantics used by Space.fromString and PendingMovement.fromContract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 674a3c68-4a76-4265-a1a6-d074198203f1
📒 Files selected for processing (43)
AGENTS.mdsrc/Imperium.Terminal/Accounting/Host.fssrc/Imperium.Terminal/Rondel/Host.fssrc/Imperium/Accounting.fssrc/Imperium/Accounting.fsisrc/Imperium/Accounting/Accounting.fssrc/Imperium/Accounting/Accounting.fsisrc/Imperium/Accounting/Commands.fssrc/Imperium/Accounting/Commands.fsisrc/Imperium/Accounting/Dependencies.fssrc/Imperium/Accounting/Dependencies.fsisrc/Imperium/Accounting/Events.fssrc/Imperium/Accounting/Events.fsisrc/Imperium/Accounting/Handlers.fssrc/Imperium/Contract/Accounting.fssrc/Imperium/Contract/Contract.fssrc/Imperium/Contract/Rondel.fssrc/Imperium/Gameplay/Gameplay.fssrc/Imperium/Gameplay/Gameplay.fsisrc/Imperium/Imperium.fsprojsrc/Imperium/Primitives/AsyncExtensions.fssrc/Imperium/Primitives/Primitives.fssrc/Imperium/Rondel.fssrc/Imperium/Rondel.fsisrc/Imperium/Rondel/Commands.fssrc/Imperium/Rondel/Commands.fsisrc/Imperium/Rondel/Dependencies.fssrc/Imperium/Rondel/Dependencies.fsisrc/Imperium/Rondel/Events.fssrc/Imperium/Rondel/Events.fsisrc/Imperium/Rondel/Handlers.fssrc/Imperium/Rondel/Invoices.fssrc/Imperium/Rondel/Movement.fssrc/Imperium/Rondel/Queries.fssrc/Imperium/Rondel/Rondel.fssrc/Imperium/Rondel/Rondel.fsisrc/Imperium/Rondel/State.fssrc/Imperium/Rondel/State.fsisrc/Imperium/Rondel/Types.fssrc/Imperium/Rondel/Types.fsitests/Imperium.UnitTests/Accounting.fstests/Imperium.UnitTests/Rondel.fstests/Imperium.UnitTests/RondelHostTests.fs
💤 Files with no reviewable changes (4)
- src/Imperium/Rondel.fs
- src/Imperium/Accounting.fs
- src/Imperium/Rondel.fsi
- src/Imperium/Accounting.fsi
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
Restructure
src/Imperiumfrom a flat file list undernamespace Imperiuminto one folder per bounded context, each owning its own namespace plus a[<RequireQualifiedAccess>]facade module. Follows the recommended approach (Approach A) in/tmp/research-topic.md. No behaviour or public-call-semantics change beyond the explicitRondel.execute/Accounting.executequalification.Folder layout (compile order mirrors
Imperium.fsproj):Primitives/—Primitives.fs,AsyncExtensions.fsContract/—Contract.fs,Accounting.fs,Rondel.fs(namespaceImperium.Contract)Gameplay/—Gameplay.fsi/.fs(namespaceImperium.Gameplay,[<RQA>] module Gameplay)Accounting/—Commands/Events/Dependencies/Handlers/Accountingfacade (namespaceImperium.Accounting,[<RQA>] module Accounting)Rondel/—Types/Commands/Events/State/Dependencies/Movement/Invoices/Handlers/Queries/Rondelfacade (namespaceImperium.Rondel,[<RQA>] module Rondel)Caller convention:
open Imperium.Rondel; Rondel.execute deps cmd(bareexecuteis no longer in scope; same for Accounting).Notable design decisions:
MoveCommand.fromContract,AccountingEvent.toContract, etc.) live in the same.fsi/.fspair as the type they accompany — F# disallows splitting them across files of the same assembly.Space.toString,Space.fromString,Space.distance,Action.toString,RondelBillingId.create,RondelBillingId.newId) are declared asval internalin the relevant.fsi.RondelCommand,AccountingDependencies, etc.) so terminal/web composition roots canopenmultiple bounded contexts without ambiguity.Commands,Events), state (State), dependency contracts (Dependencies), and the public facade.Commits (8):
497c325Move Imperium source files into bounded-context foldersf5dae47Migrate Accounting to its own namespace with qualified facade module96769e0Migrate Rondel to its own namespace with per-concept file split0cf0f21Migrate Gameplay to its own namespace with qualified facade modulef5421b7Document new namespace + facade layout in AGENTS.md7d71613Split domain dependency types into their own Dependencies file pair87429d0Rephrase type-companion module placement as a positive ruleeaf5803Split Commands and Events into dedicated file pairs per BCTest plan
dotnet build Imperium.slnx— 0 warnings, 0 errors after every stepdotnet test— 130 / 130 passing after every stepdotnet fantomas --check .— clean after every stepdotnet run --no-build --project tests/Imperium.UnitTests/Imperium.UnitTests.fsproj -- --render-spec-markdown— spec markdown still rendersSummary by CodeRabbit
Documentation
Refactor