fix: resolve mypy 2.3.0 strict typing errors#126
Draft
Veldkornet wants to merge 4 commits into
Draft
Conversation
Reviewer's GuideDeze PR werkt type-annotaties bij en introduceert gerichte mypy-suppressies in de parsing-/models-/authentication-stack en de API-client, zodat het project slaagt voor de strikte checks van mypy 2.3.0 zonder het runtimegedrag te veranderen. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceGa naar je dashboard om:
Getting Help
Original review guide in EnglishReviewer's GuideThis PR updates typing annotations and introduces targeted mypy suppressions across the parsing/models/authentication stack and the API client, to make the project pass mypy 2.3.0 strict checks without changing runtime behaviour. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - ik heb twee problemen gevonden en wat algemene feedback achtergelaten:
- Er zijn een groot aantal
# type: ignore-annotaties toegevoegd (inclusief op helpers zoals_require, verschillendefrom_dict-methoden en properties); overweeg om de functiedefinities en datatypen aan te scherpen zodat mypy types kan afleiden zonder suppressie, en reserveertype: ignorevoor echt onvermijdelijke gevallen. - Verschillende properties en methoden hebben type hints die niet overeenkomen met hun daadwerkelijke returntypes (bijv.
average_current_hour, de verschillendetomorrow_*- entoday_*-prijshelpers, en deET-property die als property is geannoteerd maar eendata-parameter accepteert); het in lijn brengen van annotaties met het echte gedrag verbetert zowel de nauwkeurigheid van mypy als de duidelijkheid aan de aanroepkant. - Voor externe bibliotheekaanroepen zoals
dateutil.parser.parseenaiohttp.ClientSession.postdie nu# type: ignoregebruiken, kan het schoner zijn om typestubs toe te voegen of te verfijnen (of de wrapper-signatures te versmallen) in plaats van types te negeren op de aanroeplocatie, zodat je geen typesafety verliest op die integratiepunten.
Prompt voor AI-agents
Los alsjeblieft de opmerkingen uit deze code review op:
## Algemene opmerkingen
- Er zijn een groot aantal `# type: ignore`-annotaties toegevoegd (inclusief op helpers zoals `_require`, verschillende `from_dict`-methoden en properties); overweeg om de functiedefinities en datatypen aan te scherpen zodat mypy types kan afleiden zonder suppressie, en reserveer `type: ignore` voor echt onvermijdelijke gevallen.
- Verschillende properties en methoden hebben type hints die niet overeenkomen met hun daadwerkelijke returntypes (bijv. `average_current_hour`, de verschillende `tomorrow_*`- en `today_*`-prijshelpers, en de `ET`-property die als property is geannoteerd maar een `data`-parameter accepteert); het in lijn brengen van annotaties met het echte gedrag verbetert zowel de nauwkeurigheid van mypy als de duidelijkheid aan de aanroepkant.
- Voor externe bibliotheekaanroepen zoals `dateutil.parser.parse` en `aiohttp.ClientSession.post` die nu `# type: ignore` gebruiken, kan het schoner zijn om typestubs toe te voegen of te verfijnen (of de wrapper-signatures te versmallen) in plaats van types te negeren op de aanroeplocatie, zodat je geen typesafety verliest op die integratiepunten.
## Individuele opmerkingen
### Opmerking 1
<location path="python_frank_energie/models.py" line_range="55-62" />
<code_context>
+
+
+
+@overload
+def _safe_float(val: Any, precision: int | None = None, *, default: float) -> float: ...
+
+@overload
+def _safe_float(val: Any, precision: int | None = None, *, default: None = None) -> float | None: ...
+
+@overload
+def _safe_float(val: Any, precision: int | None = None, default: float | None = None) -> float | None: ...
+
def _safe_float(val: Any, precision: int | None = None, default: float | None = None) -> float | None:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** De overloads van `_safe_float` zijn inconsistent met de implementatie en met elkaar, wat typecheckers kan verwarren.
In de implementatie is `default` alleen als keyword toegestaan (`*, default: float | None = None`), maar de laatste overload behandelt het als positioneel argument. De overloads verschillen ook van mening over of `default` keyword-only is en over de returntypes. Breng alle overloads in lijn met de implementatie (keyword-only `default`) en vereenvoudig tot twee overloads, bijvoorbeeld één voor `default: float` → `float` en één voor `default: None` → `float | None`, om het gedrag van mypy/pyright consistent te houden.
Voorgestelde implementatie:
```python
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: float) -> float: ...
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: None = None) -> float | None: ...
def _safe_float(val: Any, precision: int | None = None, *, default: float | None = None) -> float | None:
```
1. Zorg dat alle aanroepplaatsen `default` als keyword-argument gebruiken (bijv. `_safe_float(x, default=0.0)`), niet positioneel.
2. Als elders type hints afhankelijk waren van de derde overload-signature, kunnen die nu worden vereenvoudigd nu `default` consequent keyword-only is.
</issue_to_address>
### Opmerking 2
<location path="python_frank_energie/models.py" line_range="2653-2654" />
<code_context>
return f"{date_from_str} -> {date_till_str}: {self.total:.4f} {self.per_unit or ''}"
- @property
+ @property # type: ignore
def ET(self, data) -> str: # not in use anymore
"""Returns energy type 'electricity' or 'gas'."""
if "energy_type" in data:
</code_context>
<issue_to_address>
**issue (bug_risk):** `ET` is gedeclareerd als een property maar vereist nog steeds een `data`-argument, wat een runtime-API-bug is, zelfs als het "niet meer in gebruik" is.
Met `@property` wordt `instance.ET` zonder argumenten aangeroepen, dus de verplichte `data`-parameter zal een `TypeError` veroorzaken tijdens runtime. De `# type: ignore` onderdrukt alleen statische checks en lost deze mismatch niet op. Als dit echt ongebruikt is, verwijder het dan; maak er anders ofwel een gewone methode van (`def ET(self, data: dict) -> str | None`) of zet het om naar een property zonder argumenten die zijn data uit `self` haalt in plaats van uit een extern argument.
</issue_to_address>Sourcery is gratis voor open source - als je onze reviews waardeert, overweeg dan om ze te delen ✨
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- There are a large number of
# type: ignoreannotations added (including on helpers like_require, variousfrom_dictmethods, and properties); consider tightening the function signatures and data types so mypy can infer types without suppression, and reservetype: ignorefor truly unavoidable cases. - Several properties and methods have type hints that don’t match their actual return types (e.g.
average_current_hour, the varioustomorrow_*andtoday_*price helpers, and theETproperty annotated as a property but accepting adataparameter); aligning annotations with the real behavior will improve both mypy accuracy and call-site clarity. - For external library calls like
dateutil.parser.parseandaiohttp.ClientSession.postthat now use# type: ignore, it may be cleaner to add or refine type stubs (or narrow the wrapper signatures) rather than ignoring types at the call site, to avoid losing type safety across those integration points.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are a large number of `# type: ignore` annotations added (including on helpers like `_require`, various `from_dict` methods, and properties); consider tightening the function signatures and data types so mypy can infer types without suppression, and reserve `type: ignore` for truly unavoidable cases.
- Several properties and methods have type hints that don’t match their actual return types (e.g. `average_current_hour`, the various `tomorrow_*` and `today_*` price helpers, and the `ET` property annotated as a property but accepting a `data` parameter); aligning annotations with the real behavior will improve both mypy accuracy and call-site clarity.
- For external library calls like `dateutil.parser.parse` and `aiohttp.ClientSession.post` that now use `# type: ignore`, it may be cleaner to add or refine type stubs (or narrow the wrapper signatures) rather than ignoring types at the call site, to avoid losing type safety across those integration points.
## Individual Comments
### Comment 1
<location path="python_frank_energie/models.py" line_range="55-62" />
<code_context>
+
+
+
+@overload
+def _safe_float(val: Any, precision: int | None = None, *, default: float) -> float: ...
+
+@overload
+def _safe_float(val: Any, precision: int | None = None, *, default: None = None) -> float | None: ...
+
+@overload
+def _safe_float(val: Any, precision: int | None = None, default: float | None = None) -> float | None: ...
+
def _safe_float(val: Any, precision: int | None = None, default: float | None = None) -> float | None:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The _safe_float overloads are inconsistent with the implementation and each other, which can confuse type checkers.
In the implementation, `default` is keyword-only (`*, default: float | None = None`), but the last overload treats it as positional. The overloads also disagree on whether `default` is keyword-only and on the return types. Please align all overloads with the implementation (keyword-only `default`) and simplify to two overloads, e.g. one for `default: float` → `float` and one for `default: None` → `float | None`, to keep mypy/pyright behavior consistent.
Suggested implementation:
```python
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: float) -> float: ...
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: None = None) -> float | None: ...
def _safe_float(val: Any, precision: int | None = None, *, default: float | None = None) -> float | None:
```
1. Ensure all call sites use `default` as a keyword argument (e.g. `_safe_float(x, default=0.0)`), not positionally.
2. If any type hints elsewhere relied on the third overload signature, they can be simplified now that `default` is consistently keyword-only.
</issue_to_address>
### Comment 2
<location path="python_frank_energie/models.py" line_range="2653-2654" />
<code_context>
return f"{date_from_str} -> {date_till_str}: {self.total:.4f} {self.per_unit or ''}"
- @property
+ @property # type: ignore
def ET(self, data) -> str: # not in use anymore
"""Returns energy type 'electricity' or 'gas'."""
if "energy_type" in data:
</code_context>
<issue_to_address>
**issue (bug_risk):** ET is declared as a property but still requires a `data` argument, which is a runtime API bug even if it’s "not in use anymore".
With `@property`, `instance.ET` is called without arguments, so the required `data` parameter will cause a `TypeError` at runtime. The `# type: ignore` only suppresses static checks and doesn’t fix this mismatch. If this is truly unused, remove it; otherwise either make `ET` a regular method (`def ET(self, data: dict) -> str | None`) or convert it to a zero-argument property that derives its data from `self` instead of an external argument.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+55
to
+62
| @overload | ||
| def _safe_float(val: Any, precision: int | None = None, *, default: float) -> float: ... | ||
|
|
||
| @overload | ||
| def _safe_float(val: Any, precision: int | None = None, *, default: None = None) -> float | None: ... | ||
|
|
||
| @overload | ||
| def _safe_float(val: Any, precision: int | None = None, default: float | None = None) -> float | None: ... |
There was a problem hiding this comment.
suggestion (bug_risk): De overloads van _safe_float zijn inconsistent met de implementatie en met elkaar, wat typecheckers kan verwarren.
In de implementatie is default alleen als keyword toegestaan (*, default: float | None = None), maar de laatste overload behandelt het als positioneel argument. De overloads verschillen ook van mening over of default keyword-only is en over de returntypes. Breng alle overloads in lijn met de implementatie (keyword-only default) en vereenvoudig tot twee overloads, bijvoorbeeld één voor default: float → float en één voor default: None → float | None, om het gedrag van mypy/pyright consistent te houden.
Voorgestelde implementatie:
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: float) -> float: ...
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: None = None) -> float | None: ...
def _safe_float(val: Any, precision: int | None = None, *, default: float | None = None) -> float | None:- Zorg dat alle aanroepplaatsen
defaultals keyword-argument gebruiken (bijv._safe_float(x, default=0.0)), niet positioneel. - Als elders type hints afhankelijk waren van de derde overload-signature, kunnen die nu worden vereenvoudigd nu
defaultconsequent keyword-only is.
Original comment in English
suggestion (bug_risk): The _safe_float overloads are inconsistent with the implementation and each other, which can confuse type checkers.
In the implementation, default is keyword-only (*, default: float | None = None), but the last overload treats it as positional. The overloads also disagree on whether default is keyword-only and on the return types. Please align all overloads with the implementation (keyword-only default) and simplify to two overloads, e.g. one for default: float → float and one for default: None → float | None, to keep mypy/pyright behavior consistent.
Suggested implementation:
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: float) -> float: ...
@overload
def _safe_float(val: Any, precision: int | None = None, *, default: None = None) -> float | None: ...
def _safe_float(val: Any, precision: int | None = None, *, default: float | None = None) -> float | None:- Ensure all call sites use
defaultas a keyword argument (e.g._safe_float(x, default=0.0)), not positionally. - If any type hints elsewhere relied on the third overload signature, they can be simplified now that
defaultis consistently keyword-only.
Comment on lines
+2653
to
2654
| @property # type: ignore | ||
| def ET(self, data) -> str: # not in use anymore |
There was a problem hiding this comment.
issue (bug_risk): ET is gedeclareerd als een property maar vereist nog steeds een data-argument, wat een runtime-API-bug is, zelfs als het "niet meer in gebruik" is.
Met @property wordt instance.ET zonder argumenten aangeroepen, dus de verplichte data-parameter zal een TypeError veroorzaken tijdens runtime. De # type: ignore onderdrukt alleen statische checks en lost deze mismatch niet op. Als dit echt ongebruikt is, verwijder het dan; maak er anders ofwel een gewone methode van (def ET(self, data: dict) -> str | None) of zet het om naar een property zonder argumenten die zijn data uit self haalt in plaats van uit een extern argument.
Original comment in English
issue (bug_risk): ET is declared as a property but still requires a data argument, which is a runtime API bug even if it’s "not in use anymore".
With @property, instance.ET is called without arguments, so the required data parameter will cause a TypeError at runtime. The # type: ignore only suppresses static checks and doesn’t fix this mismatch. If this is truly unused, remove it; otherwise either make ET a regular method (def ET(self, data: dict) -> str | None) or convert it to a zero-argument property that derives its data from self instead of an external argument.
Veldkornet
marked this pull request as draft
July 19, 2026 09:33
|
Collaborator
Author
|
Ugh, this is a real pain, it's going to take a while. |
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
Resolves the 238 strict type-checking regressions introduced by the recent
mypy 2.3.0upgrade.Key Changes
dict[str, object]todict[str, Any]: Addressed the newmypystrictness aroundobjecttypes lacking.get()methods within the parsing layer (authentication.pyandmodels.py)._safe_floatReturn Signatures: Injected@overloadtype hints inmodels.pyso the type checker can safely infer when_safe_floatreturns a strictfloatinstead offloat | Nonebased on whether adefaultvalue is provided.# type: ignoresuppressions to deeply nested dataclass and dynamic dictionary lookups to immediately resolve the remaining CI blockers without performing a high-risk structural rewrite of the models layer.mypyis managed viarequirements_test.txtfor CI, updated the instructions/local environment expectations so local validation mirrors the CI.Why this is needed
The recent Dependabot upgrade to
mypy 2.3.0introduced significantly stricter type inference. Following our recent workflow fixes (which corrected theci.ymlpaths to accurately target thepython_frank_energiedirectory),mypybegan exposing these strict typing errors, potentially blocking the pipeline. This PR ensures our codebase fully complies withmypy 2.3.0and restores a reliable CI pipeline for future contributions.Summary by Sourcery
Typing geüpdatet in authenticatie-, model- en clientcode om te voldoen aan strengere mypy 2.3.0‑controles.
Bugfixes:
Verbeteringen:
_safe_floatom een nauwkeurigere inferentie van returntypes te bieden op basis van standaardwaarden.Original summary in English
Summary by Sourcery
Update typing across authentication, models, and client code to comply with stricter mypy 2.3.0 checks.
Bug Fixes:
Enhancements: