Intis/review-guidelines

Fork 0

Files

Eden Kirin 79abebff51 first commit

2026-03-06 14:50:30 +01:00

17 KiB

Raw Permalink Blame History

General Review Guidelines

Derived from real code review history across three services:

planogrammer — 128 MRs, 954 comments
cashtrack — 61 MRs, 238 comments
assetor — 28 MRs, 195 comments

Rules confirmed across multiple services are marked with the services that evidence them. Newer reviews take precedence over older ones within each service.

Code Style & Naming

DO name variables to reflect their precise meaning. The name should answer "what is this a collection of?" Prefer non_existing_ids over missing, changed_column_ids over changes. (planogrammer)
DO name boolean variables so they read as a statement:
```
is_changed = column.id in changed_column_ids
```
(planogrammer)

DO simplify boolean assignments. Instead of an if/else block that sets True/False:

# Before
if source == PlanogramChangeRequestSource.AI:
    machine.planogram_optimized_by_ai = True
else:
    machine.planogram_optimized_by_ai = False

# After
machine.planogram_optimized_by_ai = source == PlanogramChangeRequestSource.AI

(planogrammer)

DO decompose complex boolean comparisons into named intermediate variables:

are_products_equal = layout_column.product is None or layout_column.product == machine_column.product
are_recipes_equal  = layout_column.recipe is None  or layout_column.recipe  == machine_column.recipe
return are_products_equal and are_recipes_equal and ...

(planogrammer)

DO use the X | None syntax for optional type hints:

column_filter: PushPlanogramColumnFilterDto | None = None

(planogrammer)

DO use Iterable[T] for input parameters that only need to be iterated, not List[T]. Accept the most general type; return the most specific type. (assetor)

DO use typed dataclasses for internal structured return values and error payloads rather than plain dicts or multi-value tuples:

# AVOID
(asset_per_device_id, company_brand_ids, failed_responses) = await manager.bulk_create_preload(...)
failed_responses.append({"device_id": device.id, "reason": "..."})

# DO instead
@dataclass
class BulkPreloadResult:
    asset_per_device_id: dict
    failed_responses: list[FailedResponse]

@dataclass
class FailedResponse:
    device_id: int
    reason: str

Consider static factory methods for repeated reason strings: FailedResponse.for_device_already_has_asset(device_id). (assetor)

DO prefix limit/capacity constants with MAX_, e.g. MAX_GET_PRODUCT_BULK_IDS = 1000. This signals that the constant is an upper bound, not a config value. (planogrammer)
DO suffix all custom exception classes with Exception, e.g. UnableToDeleteColumnException, BulkLimitExceededException. (planogrammer)
DO use single _ prefix for protected helpers, double __ prefix for private module-level functions (name mangling). The convention is: _ = protected, __ = private. (planogrammer)

DO prefer named-constant arithmetic for time durations:

SHORT_PRICE_REVERT_PERIOD = 24 * HOURS  # readable
# not: 86400                            # magic number
# not: 60 * 60 * 24                     # still unclear at a glance

(planogrammer)

DO extract repeated small operations into named helper functions. This removes duplication and makes the calling code read at a consistent level of abstraction:
```
def _clear_changed_prices(column): ...
def _get_price_or_none(price): ...
```
(planogrammer)

DO invert conditionals to enable early-return/continue patterns:

if not is_changed:
    _clear_changed_prices(column)
    continue
# handle the positive case at normal indentation

(planogrammer)

AVOID generic update helpers that accept Any. A signature like _update_field(name: str, value: Any) eliminates type-checking. Use individual named methods:

# AVOID
def _update_field(self, name: str, value: Any) -> None:
    setattr(self._conform, name, value)

# DO instead
def update_status(self, status: ConformStatus) -> None: ...
def update_note(self, note: str) -> None: ...

(cashtrack)

DO include the type in field names when multiple variants exist or may be added:

# AVOID — breaks when value_token_diff is added later
token_diff: Decimal

# DO instead
vend_token_diff: Decimal

(cashtrack)

DO keep constants (filter lists, limits, defaults) in a dedicated const.py rather than inline in query or view code. (cashtrack)
DO add schema descriptions for non-obvious query parameters. (cashtrack)
DO order method parameters hierarchically: tenant_id before company_id, more general before more specific. Apply consistently across all call sites. (assetor)
AVOID datetime.utcnow() in new code. Use datetime.now(UTC) — utcnow() is deprecated in Python 3.12+. (assetor, planogrammer: use project utility module)
AVOID using assert in production/runtime code. Assertions are stripped with -O. Raise an explicit exception instead. (planogrammer)
AVOID tabs; use spaces. Run ruff format before pushing. Install pre-commit hooks so formatting is enforced automatically on every commit. (planogrammer, cashtrack, assetor)
AVOID broad ruff/mypy ignore rules in pyproject.toml. Verify each suppression is genuinely needed by running pre-commit run --all-files. (assetor)

API Design

DO place each new domain in its own folder with its own endpoints.py, manager.py, dto.py, and exceptions.py. Don't embed new domain read logic in an existing domain's folder. (planogrammer)

DO add an upper-bound validation on all bulk list inputs:

MAX_GET_PRODUCT_BULK_IDS = 1000
if len(ids) > MAX_GET_PRODUCT_BULK_IDS:
    raise BulkLimitExceededException(...)

(planogrammer, cashtrack)

DO enforce input length limits at the API layer to match DB column constraints:
- note fields: max 500 chars
- barcode fields: max 128 chars (cashtrack)
DO use request body (not query params) for mutation inputs. (cashtrack)
DO default time-bounded list endpoints to a sensible window when date params are omitted (e.g. last 90 days). Document the default in the endpoint description. (cashtrack)
DO make parameters mandatory when they are always required for business logic. Optional parameters that are always supplied create false API flexibility. (planogrammer)

DO use mutually exclusive validation when exactly one of several filters must be provided:

@model_validator(mode="after")
def validate_exclusive_filter(self) -> "PushFilterDto":
    provided = sum([bool(self.ids), bool(self.columns), bool(self.view_columns)])
    if provided != 1:
        raise ValueError("Exactly one filter must be provided")
    return self

(planogrammer, cashtrack)

DO use the correct HTTP status codes:
- 404 when a referenced resource is not found (not 400 or 500) (assetor)
- 409 Conflict for business-rule violations unrelated to authorization (not 403) (planogrammer)
DO for POST/PUT/PATCH/DELETE endpoints, instantiate the repository directly in the endpoint function rather than injecting it as a FastAPI dependency. This ensures transactions roll back correctly on exception. (planogrammer)
DO use company_exists(company_id) when you only need to verify existence; reserve get_company for when you need the object's fields. (planogrammer)
DO write query parameter help text in imperative form. Don't expose internal implementation details (e.g. which database is queried) in OpenAPI docs. (planogrammer)
DO keep success responses for void operations minimal — a short "ok" or empty 204, not a verbose success DTO. (planogrammer)
DO use SQLAlchemy 2.x style in all new queries:
- select(Model) not select([Model])
- (condition, value) not [(condition, value)] in CASE expressions
- Add .mappings() to raw SQL results for dictionary-like access (assetor)
AVOID kebab-case query parameter names when the rest of the service uses snake_case. (planogrammer)

Error Handling & Exceptions

DO place domain-specific exceptions in that domain's own exceptions.py. If an exception is only raised within one flow, it belongs in that flow's module. (planogrammer)
DO add a catch-all handler in every endpoint for the base exception class of your domain exceptions. This ensures unhandled domain errors return a structured response rather than 500. (planogrammer)

DO narrow try/except blocks to only the lines that can raise the caught exception:

# AVOID — business logic inside the try block catches unintended exceptions
try:
    assignment = await self._get_asset_machine(asset)
    if assignment.machine_id is None:
        raise AssetItemNotAssigned(asset.id)
except ObjectNotFound:
    raise AssetItemNotAssigned(asset.id)

# DO instead
try:
    assignment = await self._get_asset_machine(asset)
except ObjectNotFound:
    raise AssetItemNotAssigned(asset.id)

if assignment.machine_id is None:
    raise AssetItemNotAssigned(asset.id)

(assetor)

DO log the caught exception (at least at DEBUG level) when swallowing or transforming it, so the original cause remains traceable in logs. (planogrammer)
DO validate payloads thoroughly before persisting:
- Reject if all denomination/token values are zero
- Reject duplicate entries in the same payload
- Validate numeric values against DB column constraints before hitting the DB (cashtrack)
DO handle external service unavailability explicitly. Always add error handling for the unreachable case when calling external services. (assetor)
DO skip unknown enum/field values in changelog and audit endpoints rather than raising. (cashtrack)
AVOID raising a generic Exception for domain-specific error conditions. Define a named exception class. (planogrammer)
AVOID importing the entire exceptions module — import only the specific class you need. (planogrammer)

Testing

DO write tests in the same MR as the feature. (cashtrack, planogrammer)
DO assert on IDs only when verifying list/collection responses, not on full DTO content:
```
assert {r["id"] for r in response_json["content"]} == {p1.id, p2.id}
```
This prevents test breakage when DTO fields are added or renamed. (planogrammer)
DO put manager-level behaviour tests in test_<domain>_manager.py, not in endpoint tests. Endpoint tests should only exercise endpoint-layer code. (assetor)
DO cover every status transition with a dedicated test. (assetor, cashtrack)
DO cover filter behaviour at the endpoint level, including interactions with data-access restrictions. (cashtrack)
DO add tests for prices with 4 decimal places — this edge case has caused production bugs. (planogrammer)

DO write a serialization test for every response DTO that contains Decimal or datetime fields:

def test_when_model_dump_with_json_mode_then_price_is_float():
    dto = MyResponseDto(price=Decimal("1.2345"))
    result = json.loads(dto.model_dump_json())
    assert isinstance(result["price"], float)

(planogrammer)

DO use standard setup_method / teardown_method for synchronous test setup/teardown. Only use async fixtures when you genuinely need to await inside them. (planogrammer)
AVOID hard-coding specific auto-increment IDs in test assertions. Use the IDs from the objects created in test setup. (planogrammer)

Architecture & Domain Rules

DO fetch company/entity once per request and pass the object downstream. Never call get_company (or equivalent) more than once in the same request path. (planogrammer)
DO move entity existence checks into manager/service methods, not the endpoint layer. (planogrammer)
DO batch multiple individual DB queries into a single list query where possible. (planogrammer)
DO decompose large methods that do multiple conceptually distinct things. One method should do one thing. (planogrammer, cashtrack)
DO keep manager responsibilities focused on a single flow. Read/GET logic and write logic belong in separate managers. (planogrammer)
DO initialize collaborating managers in the constructor, not lazily inside methods. (planogrammer)
DO use save_many(entities) for bulk saves instead of looping save(entity). (planogrammer)
DO pass already-loaded ORM objects to methods rather than re-fetching by ID. (planogrammer)
DO prefer explicit save() calls over relying on implicit ORM dirty-tracking. Explicit saves make the persistence boundary visible. (planogrammer)
DO prefer a direct field-update query over a fetch-then-modify-then-save pattern when updating a single field. (planogrammer)
DO use soft-delete (alive flag or equivalent) to preserve historical records. Always filter queries with the soft-delete sentinel — omitting it causes deleted records to leak into results. (cashtrack)
DO look up by natural key before creating on import operations. Reuse an existing record if it already exists rather than always inserting. (cashtrack)
DO recalculate derived totals (e.g. total_coins, total_bills) from source data whenever source data changes — never accept pre-calculated totals from the caller. (cashtrack)
DO wire data-access (company + user) restrictions into every new list or filter endpoint before the MR is ready for review. (cashtrack)
DO resolve circular imports using TYPE_CHECKING guards and from __future__ import annotations:
```
from __future__ import annotations
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from televend_core.databases.televend_repositories.asset_item.model import AssetItem
```
Use @dataclass (stdlib) rather than Pydantic for internal DTOs when forward references cause Pydantic config errors. (assetor)
AVOID nesting context managers from different sources. Use one transaction boundary per operation. (planogrammer)
AVOID the deprecated load_options parameter on repository calls. Use typed LoadOptions classes from televend-core. (planogrammer)
AVOID generalizing too early. If two similar pieces of code would diverge with the next requirement, keep them separate (Rule of Three). (planogrammer)

DTO & Serialization

DO implement from_model(cls, orm_model) as a classmethod on every response DTO. Keep endpoint handlers clean — don't inline model-to-DTO mapping there:
```
@classmethod
def from_model(cls, m: MachinePCR) -> "MachinePCRDto":
    return cls(id=m.id, status=m.status, ...)
```
(planogrammer)
DO use model_dump_json() (not model_dump()) when serializing DTOs that contain Decimal or datetime fields, to ensure correct JSON types. (planogrammer)
DO use ConfigDict consistently in all Pydantic v2 models. Don't mix old class Config style with model_config = ConfigDict(...) in the same codebase. (planogrammer)
DO give optional Pydantic fields an explicit default=None:
```
class MyDto(BaseModel):
    filter: str | None = None
```
(planogrammer)

Git & Process

DO name feature branches feature/CLOUD-NNNNN-short-description. The feature/ prefix is required for CI/CD pipeline triggers. (planogrammer, cashtrack, assetor)
DO title MRs as CLOUD-NNNNN: description (imperative, present tense). Avoid the "Resolve" prefix auto-generated by GitLab. (cashtrack)
DO include the full endpoint path in CHANGELOG entries:
- Bad: Added new endpoint
- Good: POST /v1/tenants/{tenant_id}/companies/{company_id}/planogram-change-requests/push/bulk (planogrammer)
DO keep MRs focused on a single ticket. Preliminary or follow-up changes belong in their own MR. (cashtrack)
DO rebase your feature branch on develop before requesting review. (planogrammer)
DO resolve all open review threads before merging. (cashtrack, planogrammer)
DO ensure ruff passes before requesting review — a cleanup-only "Ruff is happy" MR is a sign that formatting discipline slipped. (cashtrack, assetor, planogrammer)
DO add /.venv to .gitignore. (assetor)
DO bump the service version in pyproject.toml as part of any dependency-upgrade MR. (assetor)
DO update README.md when performing a major runtime upgrade. (assetor)
AVOID event_loop fixture overrides in conftest.py — no longer required with modern pytest-asyncio. (assetor)
AVOID [ci-skip] commits for changelog-only changes unless the change truly has no code impact. (planogrammer)

17 KiB Raw Permalink Blame History

General Review Guidelines

Code Style & Naming

API Design

Error Handling & Exceptions

Testing

Architecture & Domain Rules

DTO & Serialization

Git & Process

17 KiB

Raw Permalink Blame History