docs: update coding standards#19
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Updates the repository’s coding standards documentation to reflect the current seedwork patterns (DDD/hexagonal layering, command/query handling, domain event coordination, integration events, background tasks, and wiring conventions).
Changes:
- Reworks the Python baseline and Do/Don’t guidance (ports via
Protocol, handler naming, bus stack, deferred domain-event coordination). - Expands the document with end-to-end examples for aggregates, domain events, command/query handlers, integration events, background tasks, and repository wiring.
- Adds/updates naming and folder structure conventions and testing InMemory/spy guidance.
Comments suppressed due to low confidence (2)
docs/coding-standards.md:204
- In the command example,
__post_init__raisesValueError.RegistryCommandBusonly convertsDomainErrorintoResult.failed; aValueErrorwill bubble out as an unhandled exception. Update the example (and guidance) to raise aDomainErrorsubclass for domain/validation failures if you expect failures to be represented asResult.failed.
def __post_init__(self) -> None:
if self.initial_balance < 0:
raise ValueError("initial_balance must be non-negative")
docs/coding-standards.md:200
Command(andQuery) in this seedwork are defined as@dataclass(frozen=True, kw_only=True), and the examples indocs/examplesalso usekw_only=True. This example omitskw_only=True, which makes the snippet inconsistent with the established pattern in this repo's examples.
@dataclass(frozen=True)
class OpenAccountCommand(Command):
account_id: str
initial_balance: float
currency: str
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (4)
docs/coding-standards.md:99
- The aggregate example uses
@dataclass(..., eq=True)implicitly. In seedwork,Entity/AggregateRootprovide identity-based equality; leavingeqas default will generate a dataclass__eq__on the subclass and override identity semantics. Seteq=Falseon aggregate/entity dataclasses (as seedwork does).
```python
from __future__ import annotations
from dataclasses import dataclass, field
from uuid import UUID
docs/coding-standards.md:107
BankAccount.open()constructsAccountOpenedPayload(initial_balance=...), but the payload type shown later includes bothinitial_balanceandcurrency. As written, this example wouldn’t type-check/run; align the factory signature and payload construction with the payload dataclass fields (and remove unused imports likefieldif not needed).
from seedwork.domain import AggregateRoot
@dataclass(frozen=True, kw_only=True)
class BankAccount(AggregateRoot[BankAccountId]):
owner_id: BankAccountId
balance: int
@classmethod
docs/coding-standards.md:186
- This section refers to
DomainException, but the seedwork defines/catchesDomainError(there is noDomainExceptiontype).RegistryCommandBuscatchesDomainErrorspecifically and converts it toResult.failed, so the terminology and behavior described here should be updated accordingly.
- `Protocol` with `Repository[BankAccountId, BankAccount]` as base — structural typing. The method signatures (`find_by_id`, `save`, `delete_by_id`) are inherited from `Repository`; no need to redeclare them.
- `pass` body is intentional: inheriting `Repository[TId, TAgg]` already carries the full contract. Redeclaring methods is redundant and couples the port to the base class's naming.
- Returns domain types, never ORM models.
docs/coding-standards.md:219
- The command example diverges from seedwork’s conventions and won’t work as shown: (1) seedwork commands are
@dataclass(frozen=True, kw_only=True), (2) raisingValueErrorin__post_init__won’t be converted intoResult.failed(onlyDomainErroris caught), and (3) theBankAccount.open(...)call here doesn’t match the aggregate factory signature shown above (missing required args / differing types).
- Extend `DomainError`, the domain base exception type defined in the seedwork.
- The `RegistryCommandBus` catches `DomainError` and returns it in `Result.failed`.
- Keep error classes thin — message goes in the constructor argument.
---
## Application layer
### Command and handler
```python
from __future__ import annotations
from dataclasses import dataclass
from uuid import UUID
from seedwork.application import Command, CommandHandler
from seedwork.domain import DomainError
class InvalidInitialBalanceError(DomainError):
pass
- Add scope note clarifying these standards apply to consumer projects
- Entity: use Entity[TId] base (frozen, eq=False, kw_only) + validate()
- ValueObject: use ValueObject base + validate() raising DomainError subclasses
- Aggregate: add eq=False, validate(), fix AccountOpenedPayload to include currency
- Domain Events: clarify aggregate_id is a required argument (not automatic)
- Errors: fix DomainException→DomainError; show __init__ with message+code
- Command: add kw_only=True; raise DomainError subclass instead of ValueError
- Query: add Query[AccountView] type param and kw_only=True
- Integration event TYPE: align with example ("bank_account.account_opened")
- Repository impl: rename to SqlAlchemyBankAccountRepository per convention
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8efde84 to
652da57
Compare
- Add intro paragraphs and Key points / Do / Don't structure to all domain and application layer sections - Rename Errors → Domain Errors; Domain Event handler → DomainEventHandler - Add create() factory pattern for domain events with examples - Add deletion note to Domain Events section - Reorder Application layer: Integration Events and Background Tasks before Execution context and DomainEventHandler - Remove File/folder structure section - Fix Do/Don't overview table: remove stale rows, improve wording - Update CLAUDE.md: fix layer dependency attribution (Hexagonal Architecture, not DDD), correct testing patterns, add create() factory design decision Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
docs/coding-standards.md:322
- In the command handler example,
owner_idis set fromcommand.account_idand wrapped asBankAccountId(...). This is a concrete example of the ID mix-up the earlierNewTypeguidance claims to prevent; update the example to take a distinctowner_idinput (and type) or avoid settingowner_idfrom the account id.
account = BankAccount.open(
id=BankAccountId(command.account_id),
owner_id=BankAccountId(command.account_id),
initial_balance=Money(amount=command.initial_balance, currency=command.currency),
)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
docs/coding-standards.md:222
- Related to the
create()factory guidance: the “Don’t: call the constructor directly… always go through create()” rule conflicts with the repository’s current example bounded context (docs/examples/bank_account), which instantiates events via the constructor, and may confuse readers trying to follow the examples. Either update the examples to usecreate(), or adjust this section to match the existing constructor-based pattern.
- Use a `create()` classmethod as factory — callers pass plain data, the factory constructs the payload internally. This decouples the aggregate from the payload structure.
#### Don't
- Call the constructor directly from the aggregate — always go through `create()`.
…ory pattern - Introduce UserId NewType and add owner_id field to BankAccount aggregate - Update open() and reconstitute() signatures to require owner_id - Add owner_id to OpenAccountCommand and wire it through the handler - Apply create() factory classmethod to all domain event classes - Fix Entity section in coding-standards.md (owner_id: UserId, not AccountId) - Update all test call sites across docs/examples and tests/ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No description provided.