Skip to content

Commit ddd6eb4

Browse files
nabinchhaclaude
andauthored
chore: fix inaccuracies and improve AGENTS.md (#369)
* Add code organization, design principles, and test guidelines to AGENTS.md - Code organization: public before private, class method ordering, section comments for larger modules - Naming: function names must start with action verbs - Design principles: DRY, KISS, YAGNI, SOLID guidelines - Testing: parametrization, minimal fixtures, mock at boundaries, test behavior not implementation * docs: fix inaccuracies and improve AGENTS.md consistency - Update ruff version to >=0.14.10 and Python target to 3.10+ - Add missing linter rules (TID, UP006, UP007, UP045) - Document `from __future__ import annotations` as project convention - Merge duplicate Naming sections, deduplicate type annotation guidance - Clarify DRY vs KISS tension with "third occurrence" rule of thumb - Fix test example missing `Any` import - Add `make perf-import` to Common Development Tasks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: clarify class method ordering guideline in AGENTS.md Update to match actual codebase convention: dunders first, then properties, then public methods, then private helpers. Add note about grouping related method types together. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent be91adc commit ddd6eb4

1 file changed

Lines changed: 60 additions & 14 deletions

File tree

AGENTS.md

Lines changed: 60 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -130,29 +130,29 @@ make coverage # Run tests with coverage report
130130
# SPDX-License-Identifier: Apache-2.0
131131
```
132132
Use `make update-license-headers` to add headers to all files automatically.
133-
- **Imports**: Avoid importing Python modules inside method definitions. Prefer module-level imports for better performance and clarity.
134-
- **Type annotations**: ALWAYS add type annotations to all functions, methods, and class attributes (including tests).
133+
- **`from __future__ import annotations`**: Include at the top of all Python source files for deferred type evaluation.
134+
- **Imports**: Avoid importing Python modules inside method definitions. Prefer module-level imports for better performance and clarity. See [Code Style](#code-style) for detailed import and type annotation guidelines.
135135

136136
## Code Style
137137

138-
This project uses `ruff` (v0.12.3) for linting and formatting. Follow these guidelines to avoid linter errors:
138+
This project uses `ruff` (>=0.14.10) for linting and formatting. Follow these guidelines to avoid linter errors:
139139

140140
### General Formatting
141141

142142
- **Line length**: Maximum 120 characters per line
143143
- **Quote style**: Always use double quotes (`"`) for strings
144144
- **Indentation**: Use 4 spaces (never tabs)
145-
- **Target version**: Python 3.11+
145+
- **Target version**: Python 3.10+
146146

147147
### Type Annotations
148148

149-
Type annotations are REQUIRED for all code in this project. This is strictly enforced for code quality and maintainability.
149+
Type annotations are REQUIRED for all code in this project. This is strictly enforced for code quality and maintainability. Modern type syntax is enforced by ruff rules `UP006`, `UP007`, and `UP045`.
150150

151151
- **ALWAYS** add type annotations to all functions, methods, and class attributes (including tests)
152-
- Use primitive types when possible: `list` not `List`, `dict` not `Dict`, `set` not `Set`, `tuple` not `Tuple`
153-
- Use modern union syntax with `|` for optional and union types (Python 3.10+):
154-
- `str | None` not `Optional[str]`
155-
- `int | str` not `Union[int, str]`
152+
- Use primitive types when possible: `list` not `List`, `dict` not `Dict`, `set` not `Set`, `tuple` not `Tuple` (enforced by `UP006`)
153+
- Use modern union syntax with `|` for optional and union types:
154+
- `str | None` not `Optional[str]` (enforced by `UP045`)
155+
- `int | str` not `Union[int, str]` (enforced by `UP007`)
156156
- Only import from `typing` when absolutely necessary for complex generic types
157157
- For Pydantic models, use field-level type annotations
158158

@@ -173,7 +173,8 @@ Type annotations are REQUIRED for all code in this project. This is strictly enf
173173

174174
### Import Style
175175

176-
- **ALWAYS** use absolute imports, never relative imports
176+
- **ALWAYS** include `from __future__ import annotations` at the top of every Python source file (after the license header) for deferred type evaluation
177+
- **ALWAYS** use absolute imports, never relative imports (enforced by `TID`)
177178
- Place imports at module level, not inside functions (exception: it is unavoidable for performance reasons)
178179
- Import sorting is handled by `ruff`'s `isort` - imports should be grouped and sorted:
179180
1. Standard library imports
@@ -263,7 +264,7 @@ If you add a new dependency with significant import cost (>100ms):
263264
**For internal data_designer imports:**
264265

265266
```python
266-
from __future__ import annotations # Always include at top
267+
from __future__ import annotations
267268

268269
from typing import TYPE_CHECKING
269270

@@ -349,6 +350,7 @@ Follow PEP 8 naming conventions:
349350
- **Classes**: `PascalCase`
350351
- **Constants**: `UPPER_SNAKE_CASE`
351352
- **Private attributes**: prefix with single underscore `_private_var`
353+
- **Function and method names must start with an action verb**: e.g. `get_value_from` not `value_from`, `coerce_to_int` not `to_int`, `extract_usage` not `usage`
352354

353355
```python
354356
# Good
@@ -369,6 +371,32 @@ Follow PEP 8 naming conventions:
369371
pass
370372
```
371373

374+
### Code Organization
375+
376+
- **Public before private**: Public functions/methods appear before private ones in modules and classes
377+
- **Class method order**: `__init__` and other dunder methods first, then properties, then public methods, then private helpers. Group related method types together (e.g., all `@staticmethod`s in one block, all `@classmethod`s in one block).
378+
- **Prefer public over private for testability**: Use public functions (no `_` prefix) for helpers that benefit from direct testing
379+
- **Section comments in larger modules**: Use `# ---` separators to delineate logical groups (e.g. image parsing, usage extraction, generic accessors)
380+
381+
### Design Principles
382+
383+
**DRY**
384+
- Extract shared logic into pure helper functions rather than duplicating across similar call sites
385+
- Rule of thumb: tolerate duplication until the third occurrence, then extract
386+
387+
**KISS**
388+
- Prefer flat, obvious code over clever abstractions — two similar lines is better than a premature helper
389+
- When in doubt between DRY and KISS, favor readability over deduplication
390+
391+
**YAGNI**
392+
- Don't add parameters, config, or abstraction layers for hypothetical future use cases
393+
- Don't generalize until the third caller appears
394+
395+
**SOLID**
396+
- Wrap third-party exceptions at module boundaries — callers depend on canonical error types, not leaked internals
397+
- Use `Protocol` for contracts between layers
398+
- One function, one job — separate logic from I/O
399+
372400
### Common Pitfalls to Avoid
373401

374402
1. **Mutable default arguments**:
@@ -468,8 +496,12 @@ The following ruff linter rules are currently enabled (see [pyproject.toml](pypr
468496
- `I`: isort (import sorting)
469497
- `ICN`: flake8-import-conventions (standard import names)
470498
- `PIE`: flake8-pie (miscellaneous lints)
499+
- `TID`: flake8-tidy-imports (bans relative imports)
500+
- `UP006`: `List[A]` -> `list[A]`
501+
- `UP007`: `Union[A, B]` -> `A | B`
502+
- `UP045`: `Optional[A]` -> `A | None`
471503

472-
**Note**: Additional rules (E, N, UP, ANN, B, C4, DTZ, RET, SIM, PTH) are commented out but may be enabled in the future. Write code that would pass these checks for future-proofing.
504+
**Note**: Additional rules (E, N, ANN, B, C4, DTZ, RET, SIM, PTH) are commented out but may be enabled in the future. Write code that would pass these checks for future-proofing.
473505

474506
## Testing Patterns
475507

@@ -482,13 +514,23 @@ The project uses `pytest` with the following patterns:
482514
- **HTTP mocking**: pytest-httpx for mocking HTTP requests
483515
- **Coverage**: Track test coverage with pytest-cov
484516

517+
### Test Guidelines
518+
519+
- **Parametrize over duplicate**: Use `@pytest.mark.parametrize` instead of writing multiple test functions for variations of the same behavior
520+
- **Minimal fixtures**: Fixtures should be simple — one fixture, one responsibility, just setup with no behavior logic
521+
- **Shared fixtures in `conftest.py`**: Place fixtures shared across a test directory in `conftest.py`
522+
- **Mock at boundaries**: Mock external dependencies (APIs, databases, third-party services), not internal functions
523+
- **Test behavior, not implementation**: Assert on outputs and side effects, not internal call counts (unless verifying routing)
524+
- **Keep mocking shallow**: If a test requires deeply nested mocking, the code under test may need refactoring
525+
485526
Example test structure:
486527

487528
```python
488-
import pytest
529+
from typing import Any
530+
489531
from data_designer.config.config_builder import DataDesignerConfigBuilder
490532

491-
def test_something(stub_model_configs):
533+
def test_something(stub_model_configs: dict[str, Any]) -> None:
492534
"""Test description."""
493535
builder = DataDesignerConfigBuilder(model_configs=stub_model_configs)
494536
# ... test implementation
@@ -567,6 +609,10 @@ make test
567609
# Generate coverage report
568610
make coverage
569611
# View htmlcov/index.html in browser
612+
613+
# Profile import performance (use after adding heavy dependencies)
614+
make perf-import # Profile import time
615+
make perf-import CLEAN=1 # Clean cache first, then profile
570616
```
571617

572618
## Additional Resources

0 commit comments

Comments
 (0)