Skip to content

feat: Add correct types and make mypy --strict pass - #771

Open
fjovell-swish wants to merge 3 commits into
mobilityhouse:masterfrom
fjovell-swish:feature/add-type-annotations
Open

fjovell-swish wants to merge 3 commits into
mobilityhouse:masterfrom
fjovell-swish:feature/add-type-annotations

Conversation

@fjovell-swish

@fjovell-swish fjovell-swish commented Aug 28, 2026 •

Copy link
Copy Markdown

Changes included in this PR

Adds correct type annotations to the whole codebase.

Impact

Describe breaking changes, including changes a users might need to make due to this PR

Checklist

  1. Does your submission pass the existing tests?
  2. Are there new tests that cover these additions/changes?
  3. Have you linted your code locally before submission?

@fjovell-swish
fjovell-swish force-pushed the feature/add-type-annotations branch 2 times, most recently from eb15a94 to 67241b9 Compare August 28, 2026 12:51
@fjovell-swish

Copy link
Copy Markdown
Author

The only issue I found is with v201/enums.py which mypy for some reason cannot handle. To mitigate this, I excluded the files and disallowed following imports.

Reported here: python/mypy#21902

@fjovell-swish
fjovell-swish marked this pull request as ready for review August 28, 2026 12:53
@a-alak

a-alak commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Nice initiative!
I always thought types were missing.

Some considerations:

  • I really think this package need to consider which python version it will support. Python 3.9 is officially unsupported by CPython and python 3.10 will be unsupported in a month or so. It means that all supported versions support the use of list and dict to type annotate instead of importing List and Dict types. Also the use | instead of Union and Optional. Personally I think it is cleaner, so I would rather have that, but of course if python 3.9 support is necessary, it is fair. I guess eventually List, Dict, Union, Optional and more will be deprecated.

  • I ran pyrefly, pyright and ty. There is some issues with ty and pyright, and for ty it seems primarily that the error type is different, so it is not ignored (there is ignore comments). If there is support for this, I can take look to make it pass all type checkers.

  • To keep type annotation correct, type checking in CI should be considered.

@fjovell-swish
fjovell-swish force-pushed the feature/add-type-annotations branch 2 times, most recently from 7ea514d to e59e918 Compare August 31, 2026 07:38
@fjovell-swish

Copy link
Copy Markdown
Author

The only issue I found is with v201/enums.py which mypy for some reason cannot handle. To mitigate this, I excluded the files and disallowed following imports.

Reported here: python/mypy#21902

Apparently, I was wrong. There's nothing wrong with mypy, we are just reasigning the "count" identifier from a callable to a string and thus it is raising a completely valid error. Instead, I added type ignore to these lines, and removed the configuration for ignoring files, and skipping following imports.

@fjovell-swish

Copy link
Copy Markdown
Author

@a-alak While your considerations are very reasonable I think they are out of scope for this PR.

@a-alak

a-alak commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Sure, just thought those will be relevant decisions to take and PRs to make, if there is a decision to make this library support static typing.

@fjovell-swish

Copy link
Copy Markdown
Author

Certainly, not saying otherwise. At the very least, I added the mypy check into the Makefile which also checks for formatting, linting, and runs the tests.

@fjovell-swish

Copy link
Copy Markdown
Author

@mdwcrft @proelke Can you please take a look?

@mdwcrft

mdwcrft commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this, it's a solid start. I ran mypy --strict, the linters and the test suite locally on 3.11 and 3.13 and everything passes. There are a few things to address before merge though.

Blocking

1. OCPPError details dict is shared across all instances (ocpp/exceptions.py)

defailt_details: Dict[str, Any] = {} is a class attribute (also a typo for default_details), and self.details = details or self.defailt_details hands that same dict to every exception created without details, across all subclasses:

from ocpp.exceptions import NotImplementedError, FormatViolationError

e = NotImplementedError()
e.details["x"] = 1
FormatViolationError().details  # {'x': 1}

Previously each instance got a fresh {}. Suggest:

def __init__(
    self,
    description: Optional[str] = None,
    details: Optional[Dict[str, Any]] = None,
):
    self.description = (
        description if description is not None else self.default_description
    )
    self.details = details if details is not None else {}

The is not None check on description also restores the previous behaviour where an explicit description="" was kept. With description or ... it's now replaced by the default.

2. Shipping py.typed exposes types that break standard usage for downstream users

py.typed opts every consumer's type checker into these annotations, so I type-checked a minimal central system / charge point against this branch (websockets 17.1, mypy --strict):

  • WebSocket.recv() is typed -> str, but websockets' ServerConnection.recv() / ClientConnection.recv() return str | bytes, so the canonical ChargePoint("CP1", ws) fails:
    error: Argument 2 to "ChargePoint" has incompatible type "ServerConnection"; expected "WebSocket"  [arg-type]
    
    recv() -> Union[str, bytes] would fix it (json.loads in unpack accepts bytes too).
  • on() / after() return Any, so every decorated handler gets:
    error: Untyped decorator makes function "on_boot" untyped  [untyped-decorator]
    
    Typing them as Callable[[F], F] with F = TypeVar("F", bound=Callable[..., Any]) keeps the handler's own signature.
  • response_timeout: int rejects floats (e.g. response_timeout=2.5), which work fine at runtime. Should be float.

Either fix these, or hold back py.typed until the public surface is typed well enough for strict consumers.

Non-blocking

  • sys.version_info <= (3, 10) in the three enums.py files is off by one: on 3.10.x, (3, 10, 19) <= (3, 10) is False, so it takes the from enum import StrEnum branch and fails. Since pyproject.toml requires ^3.11, the simplest fix is to drop the fallback (and the coverage exclude_also) and import StrEnum directly. Otherwise it should be < (3, 11).
  • _raise_key_error is annotated -> None but always raises for the supported versions. NoReturn is more accurate (drop the trailing return), and makes the return None after its call sites in _handle_call unnecessary.
  • _is_dataclass_instance(input: DataclassInstance): typing the input as a dataclass instance defeats the purpose of the check. input: Any with -> TypeGuard[DataclassInstance] would be more useful.
  • CallError.to_exception() is annotated -> BaseException but always returns an OCPPError subclass (or raises), so -> OCPPError is more precise.
  • ChargePoint.call() returns Any, so callers get no typing from the main API. Fine for this PR, could be a follow-up (e.g. overloads keyed on the payload type).
  • Mixed annotation styles: unique_id: str | None in call() next to Optional[...] everywhere else, and unique_id is str in Call / CallResult / call() but Union[int, float, str] in _get_specific_response.

@fjovell-swish
fjovell-swish force-pushed the feature/add-type-annotations branch from e59e918 to 48d57ca Compare September 22, 2026 11:54
@fjovell-swish

Copy link
Copy Markdown
Author

Hello, @mdwcrft. Thank you for your review.

I implemented all the suggested changes in the last commit.

However, there's the issue about the py.typed file. Without it, tools like mypy are not able to pick up on the annotations when running mypy, making it not as useful as it could be otherwise.

Regarding the Any for some of the annotations: Yes, I agree should be improved and I figured that once we have this going and working when installed in projects, we can then continue working on getting rid of the remaining Any's.

@fjovell-swish
fjovell-swish force-pushed the feature/add-type-annotations branch from bcb459b to 137acf6 Compare September 22, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants