Skip to content

driver: add initial type hints for common driver code - #1946

Open
rpoisel wants to merge 7 commits into
labgrid-project:masterfrom
honeytreelabs:dev-add-typing-hints-initial
Open

driver: add initial type hints for common driver code#1946
rpoisel wants to merge 7 commits into
labgrid-project:masterfrom
honeytreelabs:dev-add-typing-hints-initial

Conversation

@rpoisel

@rpoisel rpoisel commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

This PR starts adding type annotations incrementally, as discussed in #1945.

It adds ty as a development dependency and configures it to check a small initial module of the codebase: labgrid/driver/common.py.

Verified locally with:

uv run --extra dev pytest tests/test_driver_common.py
uv run ruff check labgrid/driver/common.py tests/test_driver_common.py
uv run ruff format --check --diff labgrid/driver/common.py tests/test_driver_common.py
uv run ty check

Checklist

  • Documentation for the feature
  • Tests for the feature
  • The arguments and description in doc/configuration.rst have been updated
  • Add a section on how to use the feature to doc/usage.rst
  • Add a section on how to use the feature to doc/development.rst
  • PR has been tested
  • Man pages have been regenerated

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.0%. Comparing base (879f4d1) to head (0cd4874).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1946   +/-   ##
======================================
  Coverage    61.0%   61.0%           
======================================
  Files         182     182           
  Lines       14899   14901    +2     
======================================
+ Hits         9095    9103    +8     
+ Misses       5804    5798    -6     
Flag Coverage Δ
3.10 61.0% <100.0%> (+<0.1%) ⬆️
3.11 61.0% <100.0%> (+<0.1%) ⬆️
3.12 61.0% <100.0%> (+<0.1%) ⬆️
3.13 61.0% <100.0%> (+<0.1%) ⬆️
3.14 61.0% <100.0%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@rpoisel
rpoisel force-pushed the dev-add-typing-hints-initial branch 3 times, most recently from 2631215 to 8411023 Compare July 29, 2026 13:19
@rpoisel
rpoisel marked this pull request as ready for review July 29, 2026 13:35
Emantor
Emantor previously approved these changes Jul 31, 2026

@Emantor Emantor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, do you also want to add a ty check run to this PR for the CI or is it too early for that?

@rpoisel

rpoisel commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Do you also want to add a ty check run to this PR for the CI or is it too early for that?

Thanks for your approval!

I'd be happy to add such check to CI. If you don't mind, I'd do that in a separate commit.

EDIT: just re-pushed to get the commit of this PR verified.

@rpoisel
rpoisel force-pushed the dev-add-typing-hints-initial branch from 8411023 to 5efa0dc Compare July 31, 2026 09:24
Add `ty` as a development dependency and configure it to check one
initial driver module.

Annotate `labgrid.driver.common` while leaving existing attrs-based
exception behavior unchanged.

Signed-off-by: Rainer Poisel <rainer@embedded-focus.com>
@rpoisel
rpoisel force-pushed the dev-add-typing-hints-initial branch from 5efa0dc to 427d66f Compare July 31, 2026 09:32
@Bastian-Krause
Bastian-Krause self-requested a review August 5, 2026 21:26

@Bastian-Krause Bastian-Krause left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to add such check to CI. If you don't mind, I'd do that in a separate commit.

You didn't to that, did you?

Maybe we should drop the mypy.ini with this PR? (edit: depending on the mypy question below)

Comment thread labgrid/driver/common.py Outdated
Comment thread labgrid/driver/common.py Outdated
Comment thread labgrid/driver/common.py Outdated
Comment thread labgrid/driver/common.py Outdated
super().__attrs_post_init__()
if self.target is None:
raise BindingError("Drivers can only be created on a valid target")
raise BindingError("Drivers can only be created on a valid target") # ty: ignore[too-many-positional-arguments]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need to mark each instantiation of an attr.s class like that? I don't think that scales..

Doesn't mypy work better with that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR mypy had the same issues, thats why we never got anywhere with the initial type hints.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the separate PR that will add ty as a dependency incl. CI check, I'll disable the too-many-positional-arguments check for the whole repo until there is clean support for attrs classes.

At the moment, ty does not really support attrs. See astral-sh/ty#2404 if you want to learn more.

@rpoisel

rpoisel commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

I'd be happy to add such check to CI. If you don't mind, I'd do that in a separate commit.
You didn't to that, did you?

No, I didn't because I want to do that in a separate commit.

Maybe we should drop the mypy.ini with this PR?

I would leave it for now.

Now that you mention that: I think I will even remove the ty dependency entirely in this PR and add it in the (separate) PR that also adds the CI typing checks.

Co-authored-by: Bastian Krause <bst@pengutronix.de>
Signed-off-by: Rainer Poisel <rainer.poisel@gmail.com>
rpoisel and others added 5 commits August 7, 2026 18:32
Co-authored-by: Bastian Krause <bst@pengutronix.de>
Signed-off-by: Rainer Poisel <rainer.poisel@gmail.com>
Co-authored-by: Bastian Krause <bst@pengutronix.de>
Signed-off-by: Rainer Poisel <rainer.poisel@gmail.com>
Signed-off-by: Rainer Poisel <rainer@embedded-focus.com>
The ty type checker will be added in a subsequent PR.

Signed-off-by: Rainer Poisel <rainer@embedded-focus.com>
@rpoisel
rpoisel requested a review from Bastian-Krause August 7, 2026 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants