Skip to content

Commit 8493c3f

Browse files
BenjaminWattsBenjaminWatts
committed
fix: add pre-commit hooks and fix unit tests
- Add .pre-commit-config.yaml with black, flake8, isort, mypy, pytest - Add setup-dev.sh script for easy development setup - Fix test_client.py mock data to include required fields - Add pre-commit to requirements-dev.txt - Update GitHub Actions permissions for Pages deployment
1 parent 580f2c3 commit 8493c3f

File tree

12 files changed

+268
-1385
lines changed

12 files changed

+268
-1385
lines changed

‎.github/ISSUE_TEMPLATE/model-refinement.md‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
---
2+
name: Model Refinement
3+
about: Refine generated models to eliminate more repetition and improve type accuracy
4+
title: 'Refine generated models: additional mixins and optional field review'
5+
labels: enhancement, models, code-generation
6+
assignees: ''
7+
---
8+
9+
## Overview
10+
11+
While we've made significant improvements to the generated models (enums, mixins, snake_case, required fields), there are opportunities for further refinement.
12+
13+
## Current State
14+
15+
✅ **Completed:**
16+
- 22 enum types with 210 values
17+
- 18 field mixins (629 fields eliminated, 52.9% reduction)
18+
- 29 method mixins (~141 helper methods)
19+
- 894 required fields (64%)
20+
- 100% snake_case with aliases
21+
- Comprehensive validation
22+
23+
## Areas for Further Refinement
24+
25+
### 1. Additional Field Mixins
26+
27+
Some repeated fields could potentially be moved to mixins:
28+
29+
**Candidate fields for new mixins:**
30+
- `time` field (appears in 8 models) - could be `TimeFields` mixin
31+
- `id` field (appears in 5 models) - could be `IdFields` mixin
32+
- `metadata` field (appears in 55 wrapper models) - could be `MetadataFields` mixin
33+
- `data` field (appears in 130 wrapper models) - could be `DataFields` mixin
34+
- `bid` + `offer` fields - could be part of `BidOfferFields` mixin (currently only methods)
35+
- `margin` field (appears in 7 models) - could be `MarginFields` mixin
36+
- `surplus` field (appears in 6 models) - could be `SurplusFields` mixin
37+
38+
**Potential impact:**
39+
- Could eliminate another ~100-150 field definitions
40+
- Would increase mixin coverage from 50% to 60%+
41+
42+
### 2. Optional Field Review
43+
44+
Some fields marked as `Optional` might actually be required based on API behavior:
45+
46+
**Fields to investigate:**
47+
```python
48+
# Example from BidOfferDatasetResponse:
49+
bid: Optional[float] = Field(...) # Has example - might be required?
50+
offer: Optional[float] = Field(...) # Has example - might be required?
51+
52+
# Example from various models:
53+
time: Optional[datetime] = Field(...) # Appears in 8 models with examples
54+
```
55+
56+
**Approach:**
57+
1. Make actual API calls to test endpoints
58+
2. Analyze which fields are consistently present (>90% of responses)
59+
3. Update `inferred_required_fields` in `tools/generate_models.py`
60+
4. Regenerate models
61+
62+
**Tool available:**
63+
- `tools/infer_required_fields.py` - Tests actual API responses
64+
65+
### 3. Wrapper Model Optimization
66+
67+
Many `*_DatasetResponse` and `*_ResponseWithMetadata` models are very similar:
68+
69+
```python
70+
class SomeModel_DatasetResponse(BaseModel):
71+
data: Optional[List[SomeModel]] = None
72+
73+
class AnotherModel_DatasetResponse(BaseModel):
74+
data: Optional[List[AnotherModel]] = None
75+
```
76+
77+
**Potential solutions:**
78+
- Generic wrapper with TypeVar
79+
- Shared base class for all wrapper models
80+
- Factory function for creating wrappers
81+
82+
### 4. Additional Validations
83+
84+
Some fields could benefit from validation:
85+
86+
**Candidates:**
87+
- `frequency` - Already has FrequencyMixin, ensure all frequency fields use it
88+
- `temperature` - Already has TemperatureMixin, ensure all temperature fields use it
89+
- `percentage` fields - Validate 0-100 range
90+
- `capacity` fields - Validate positive values
91+
- `volume` fields - Could validate reasonable ranges
92+
93+
### 5. More Comprehensive Enums
94+
95+
Some string fields might have limited value sets that could be enums:
96+
97+
**Fields to investigate:**
98+
- `status` field (appears in multiple models)
99+
- `priceDirection` field
100+
- `cause` field (might have common values)
101+
- `service` field
102+
- `dataType` field
103+
104+
## Implementation Plan
105+
106+
### Phase 1: Data Collection (Low effort)
107+
- [ ] Run `tools/infer_required_fields.py` with API key
108+
- [ ] Analyze actual API responses for field presence
109+
- [ ] Document findings in `tools/field_analysis.md`
110+
111+
### Phase 2: Additional Field Mixins (Medium effort)
112+
- [ ] Create `TimeFields`, `IdFields`, `MarginFields`, `SurplusFields` mixins
113+
- [ ] Update `tools/generate_models.py` to detect these patterns
114+
- [ ] Regenerate models
115+
- [ ] Test with `test_all_improvements.py`
116+
117+
### Phase 3: Optional Field Refinement (Medium effort)
118+
- [ ] Review fields with examples that are marked Optional
119+
- [ ] Update `inferred_required_fields` based on API testing
120+
- [ ] Regenerate models
121+
- [ ] Update tests to handle new required fields
122+
123+
### Phase 4: Wrapper Model Optimization (Low-Medium effort)
124+
- [ ] Design generic wrapper approach
125+
- [ ] Implement in `tools/generate_models.py`
126+
- [ ] Test with existing code
127+
128+
### Phase 5: Additional Validations (Low effort)
129+
- [ ] Add percentage validation
130+
- [ ] Add positive value validation for capacity/volume
131+
- [ ] Ensure all frequency/temperature fields use appropriate mixins
132+
133+
## Success Criteria
134+
135+
- [ ] Field mixin coverage >60% (currently 50%)
136+
- [ ] Required field percentage >70% (currently 64%)
137+
- [ ] Additional 100-150 field definitions eliminated
138+
- [ ] All tests passing
139+
- [ ] Documentation updated
140+
141+
## Notes
142+
143+
- Maintain backward compatibility where possible
144+
- Use `populate_by_name=True` to support both snake_case and camelCase
145+
- Keep API compatibility via aliases
146+
- Ensure all changes are tested
147+
148+
## Related Files
149+
150+
- `elexon_bmrs/generated_models.py` - Generated models
151+
- `elexon_bmrs/field_mixins.py` - Field mixin definitions
152+
- `elexon_bmrs/validators.py` - Method mixin definitions
153+
- `elexon_bmrs/enums.py` - Enum definitions
154+
- `tools/generate_models.py` - Model generator
155+
- `tools/generate_enums.py` - Enum generator
156+
- `tools/infer_required_fields.py` - API testing tool
157+

‎.github/workflows/docs.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ permissions:
1414
contents: read
1515
pages: write
1616
id-token: write
17+
actions: read
1718

1819
# Allow one concurrent deployment
1920
concurrency:

‎.pre-commit-config.yaml‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
repos:
2+
- repo: https://github.com/pre-commit/pre-commit-hooks
3+
rev: v5.0.0
4+
hooks:
5+
- id: trailing-whitespace
6+
- id: end-of-file-fixer
7+
- id: check-yaml
8+
- id: check-added-large-files
9+
- id: check-json
10+
- id: check-toml
11+
- id: check-merge-conflict
12+
- id: debug-statements
13+
14+
- repo: https://github.com/psf/black
15+
rev: 25.9.0
16+
hooks:
17+
- id: black
18+
language_version: python3.11
19+
args: ['--line-length=100']
20+
21+
- repo: https://github.com/pycqa/flake8
22+
rev: 7.3.0
23+
hooks:
24+
- id: flake8
25+
args: ['--max-line-length=100', '--extend-ignore=E203,W503']
26+
27+
- repo: https://github.com/pycqa/isort
28+
rev: 6.1.0
29+
hooks:
30+
- id: isort
31+
args: ['--profile', 'black', '--line-length', '100']
32+
33+
- repo: https://github.com/pre-commit/mirrors-mypy
34+
rev: v1.18.2
35+
hooks:
36+
- id: mypy
37+
additional_dependencies: [types-requests, pydantic]
38+
args: ['--ignore-missing-imports', '--no-strict-optional']
39+
40+
- repo: local
41+
hooks:
42+
- id: pytest-check
43+
name: pytest-check
44+
entry: pytest
45+
language: system
46+
pass_filenames: false
47+
always_run: true
48+
args: ['tests/', '-v', '--tb=short']
49+
stages: [commit]
50+

‎CHANGELOG.md‎

Lines changed: 0 additions & 74 deletions
This file was deleted.

0 commit comments

Comments
 (0)