Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the py.typed marker to Python bindings #94

Closed
vfazio opened this issue Sep 11, 2024 · 8 comments
Closed

Add the py.typed marker to Python bindings #94

vfazio opened this issue Sep 11, 2024 · 8 comments

Comments

@vfazio
Copy link
Contributor

vfazio commented Sep 11, 2024

The Python bindings are currently supplying type annotations, however, without a py.typed marker, type checkers like mypy do not know to check for these annotations.

(venv) vfazio@vfazio4 /tmp/tmp.qo91wk2VVE $ cat test.py
import gpiod

chip = gpiod.Chip(1)

(venv) vfazio@vfazio4 /tmp/tmp.qo91wk2VVE $ mypy test.py
test.py:1: error: Skipping analyzing "gpiod": module is installed, but missing library stubs or py.typed marker  [import-untyped]
test.py:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

We should expect:

(venv) vfazio@vfazio4 /tmp/tmp.qo91wk2VVE $ mypy test.py
test.py:3: error: Argument 1 to "Chip" has incompatible type "int"; expected "str"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

Adding this support should be pretty easy, so I'll post a patch tomorrow

vfazio@vfazio4 ~/development/libgpiod $ git diff HEAD
diff --git a/bindings/python/gpiod/py.typed b/bindings/python/gpiod/py.typed
new file mode 100644
index 0000000..e69de29
diff --git a/bindings/python/setup.py b/bindings/python/setup.py
index 9607a28..1f04b99 100644
--- a/bindings/python/setup.py
+++ b/bindings/python/setup.py
@@ -224,6 +224,7 @@ setup(
     name="gpiod",
     url="https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git",
     packages=find_packages(exclude=["tests", "tests.*"]),
+    package_data={"gpiod": ["py.typed"]},
     python_requires=">=3.9.0",
     ext_modules=[gpiod_ext],
     cmdclass={"build_ext": build_ext, "sdist": sdist},
vfazio added a commit to vfazio/libgpiod that referenced this issue Sep 14, 2024
Per PEP 561 [0], the marker is used by type checkers like mypy
to recognize the library is typed.

[0]: https://peps.python.org/pep-0561/#packaging-type-information

Closes: brgl#94
Signed-off-by: Vincent Fazio <vfazio@gmail.com>
@vfazio
Copy link
Contributor Author

vfazio commented Sep 14, 2024

@brgl brgl closed this as completed in c840e17 Sep 16, 2024
@brgl
Copy link
Owner

brgl commented Sep 16, 2024

I applied both patches. After giving mypy a try, I'm seeing some warnings all around the place. Do you have any plans of addressing them yourself or should start looking into them?

@vfazio
Copy link
Contributor Author

vfazio commented Sep 16, 2024

I applied both patches. After giving mypy a try, I'm seeing some warnings all around the place. Do you have any plans of addressing them yourself or should start looking into them?

I can submit a single patch to address the mypy warnings. Im considering specifying some dev dependencies such as mypy and black or ruff just to keep things consistent, but that may come later

@brgl
Copy link
Owner

brgl commented Sep 16, 2024

I applied both patches. After giving mypy a try, I'm seeing some warnings all around the place. Do you have any plans of addressing them yourself or should start looking into them?

I can submit a single patch to address the mypy warnings. Im considering specifying some dev dependencies such as mypy and black or ruff just to keep things consistent, but that may come later

Ok, I'll wait for your patch then. Don't you think it would make sense to split them at least by subject?

@vfazio
Copy link
Contributor Author

vfazio commented Sep 16, 2024

I applied both patches. After giving mypy a try, I'm seeing some warnings all around the place. Do you have any plans of addressing them yourself or should start looking into them?

I can submit a single patch to address the mypy warnings. Im considering specifying some dev dependencies such as mypy and black or ruff just to keep things consistent, but that may come later

Ok, I'll wait for your patch then. Don't you think it would make sense to split them at least by subject?

Like, per file? The diff for resolving all of mypy's complaints are all over the subtree.

(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ git diff --stat
 bindings/python/gpiod/__init__.py      | 19 ++++++++++++++++++-
 bindings/python/gpiod/chip.py          | 12 +++++-------
 bindings/python/gpiod/edge_event.py    |  2 +-
 bindings/python/gpiod/info_event.py    |  2 +-
 bindings/python/gpiod/internal.py      |  3 ++-
 bindings/python/gpiod/line.py          |  2 +-
 bindings/python/gpiod/line_info.py     |  1 -
 bindings/python/gpiod/line_request.py  | 24 +++++++++++++++---------
 bindings/python/gpiod/line_settings.py |  2 +-
 9 files changed, 44 insertions(+), 23 deletions(-)
diff --git a/bindings/python/gpiod/__init__.py b/bindings/python/gpiod/__init__.py
index 9cbb8df..fa2becb 100644
--- a/bindings/python/gpiod/__init__.py
+++ b/bindings/python/gpiod/__init__.py
@@ -7,7 +7,7 @@ Python bindings for libgpiod.
 This module wraps the native C API of libgpiod in a set of python classes.
 """
 
-from . import _ext
+from . import _ext  # type: ignore[attr-defined]
 from . import line
 from .chip import Chip
 from .chip_info import ChipInfo
@@ -20,6 +20,23 @@ from .version import __version__
 
 api_version = _ext.api_version
 
+__all__ = [
+    "_ext",
+    "line",
+    "Chip",
+    "ChipInfo",
+    "EdgeEvent",
+    "ChipClosedError",
+    "RequestReleasedError",
+    "InfoEvent",
+    "LineRequest",
+    "LineSettings",
+    "is_gpiochip_device",
+    "request_lines",
+    "api_version",
+    "__version__",
+]
+
 
 def is_gpiochip_device(path: str) -> bool:
     """
diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
index 7692ba4..e7af9fd 100644
--- a/bindings/python/gpiod/chip.py
+++ b/bindings/python/gpiod/chip.py
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
-from . import _ext
+from . import _ext  # type: ignore[attr-defined]
 from .chip_info import ChipInfo
 from .exception import ChipClosedError
 from .info_event import InfoEvent
@@ -11,11 +11,9 @@ from .line_info import LineInfo
 from .line_settings import LineSettings, _line_settings_to_ext
 from .line_request import LineRequest
 from collections import Counter
-from collections.abc import Iterable
 from datetime import timedelta
 from errno import ENOENT
-from select import select
-from typing import Union, Optional
+from typing import cast, Union, Optional
 
 __all__ = "Chip"
 
@@ -56,7 +54,7 @@ class Chip:
             Path to the GPIO character device file.
         """
         self._chip = _ext.Chip(path)
-        self._info = None
+        self._info: ChipInfo | None
 
     def __bool__(self) -> bool:
         """
@@ -103,7 +101,7 @@ class Chip:
         self._check_closed()
 
         if not self._info:
-            self._info = self._chip.get_info()
+            self._info = cast(ChipInfo, self._chip.get_info())
 
         return self._info
 
@@ -226,7 +224,7 @@ class Chip:
         config: dict[tuple[Union[int, str]], Optional[LineSettings]],
         consumer: Optional[str] = None,
         event_buffer_size: Optional[int] = None,
-        output_values: Optional[dict[tuple[Union[int, str]], Value]] = None,
+        output_values: Optional[dict[Union[int, str], Value]] = None,
     ) -> LineRequest:
         """
         Request a set of lines for exclusive usage.
diff --git a/bindings/python/gpiod/edge_event.py b/bindings/python/gpiod/edge_event.py
index bf258c1..94c3ec8 100644
--- a/bindings/python/gpiod/edge_event.py
+++ b/bindings/python/gpiod/edge_event.py
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
-from . import _ext
+from . import _ext  # type: ignore[attr-defined]
 from dataclasses import dataclass
 from enum import Enum
 
diff --git a/bindings/python/gpiod/info_event.py b/bindings/python/gpiod/info_event.py
index 481eae6..a7d59c0 100644
--- a/bindings/python/gpiod/info_event.py
+++ b/bindings/python/gpiod/info_event.py
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
-from . import _ext
+from . import _ext  # type: ignore[attr-defined]
 from .line_info import LineInfo
 from dataclasses import dataclass
 from enum import Enum
diff --git a/bindings/python/gpiod/internal.py b/bindings/python/gpiod/internal.py
index 2dddb65..d2e21d3 100644
--- a/bindings/python/gpiod/internal.py
+++ b/bindings/python/gpiod/internal.py
@@ -5,10 +5,11 @@ from datetime import timedelta
 from select import select
 from typing import Optional, Union
 
-__all__ = []
+__all__ = ["poll_fd"]
 
 
 def poll_fd(fd: int, timeout: Optional[Union[timedelta, float]] = None) -> bool:
+    sec: float | None
     if isinstance(timeout, timedelta):
         sec = timeout.total_seconds()
     else:
diff --git a/bindings/python/gpiod/line.py b/bindings/python/gpiod/line.py
index d088fb4..46ca0ac 100644
--- a/bindings/python/gpiod/line.py
+++ b/bindings/python/gpiod/line.py
@@ -2,7 +2,7 @@
 # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
 
-from . import _ext
+from . import _ext  # type: ignore[attr-defined]
 from enum import Enum
 
 __all__ = ["Value", "Direction", "Bias", "Drive", "Edge", "Clock"]
diff --git a/bindings/python/gpiod/line_info.py b/bindings/python/gpiod/line_info.py
index c196a6a..715f77c 100644
--- a/bindings/python/gpiod/line_info.py
+++ b/bindings/python/gpiod/line_info.py
@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
-from . import _ext
 from dataclasses import dataclass
 from datetime import timedelta
 from gpiod.line import Direction, Bias, Drive, Edge, Clock
diff --git a/bindings/python/gpiod/line_request.py b/bindings/python/gpiod/line_request.py
index 51e600a..1d124ec 100644
--- a/bindings/python/gpiod/line_request.py
+++ b/bindings/python/gpiod/line_request.py
@@ -1,13 +1,13 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
-from . import _ext
+from . import _ext  # type: ignore[attr-defined]
 from .edge_event import EdgeEvent
 from .exception import RequestReleasedError
 from .internal import poll_fd
 from .line import Value
 from .line_settings import LineSettings, _line_settings_to_ext
-from collections.abc import Iterable
+from collections.abc import Collection
 from datetime import timedelta
 from typing import Optional, Union
 
@@ -27,6 +27,11 @@ class LineRequest:
         not part of stable API.
         """
         self._req = req
+        self._lines: list[Union[int, str]]
+        self._name_map: dict[str, int]
+        self._chip_name: str
+        self._offset_map: dict[int, str]
+        self._offsets: list[int]
 
     def __bool__(self) -> bool:
         """
@@ -86,7 +91,7 @@ class LineRequest:
         return False
 
     def get_values(
-        self, lines: Optional[Iterable[Union[int, str]]] = None
+        self, lines: Optional[Collection[Union[int, str]]] = None
     ) -> list[Value]:
         """
         Get values of a set of GPIO lines.
@@ -101,16 +106,17 @@ class LineRequest:
         """
         self._check_released()
 
-        lines = lines or self._lines
+        _lines = lines or self._lines
 
         offsets = [
-            self._name_map[line] if self._check_line_name(line) else line
-            for line in lines
+            self._name_map[line] if self._check_line_name(line) else line  # type: ignore[index]
+            for line in _lines
         ]
 
-        buf = [None] * len(lines)
+        buf: list[Value] = [None] * len(_lines)  # type: ignore[list-item]
 
         self._req.get_values(offsets, buf)
+
         return buf
 
     def set_value(self, line: Union[int, str], value: Value) -> None:
@@ -136,7 +142,7 @@ class LineRequest:
         self._check_released()
 
         mapped = {
-            self._name_map[line] if self._check_line_name(line) else line: values[line]
+            self._name_map[line] if self._check_line_name(line) else line: values[line]  # type: ignore[index]
             for line in values
         }
 
@@ -165,7 +171,7 @@ class LineRequest:
                 lines = [lines]
 
             for line in lines:
-                offset = self._name_map[line] if self._check_line_name(line) else line
+                offset = self._name_map[line] if self._check_line_name(line) else line  # type: ignore[index]
                 line_settings[offset] = settings
 
         for offset in self.offsets:
diff --git a/bindings/python/gpiod/line_settings.py b/bindings/python/gpiod/line_settings.py
index 41712cc..dab6634 100644
--- a/bindings/python/gpiod/line_settings.py
+++ b/bindings/python/gpiod/line_settings.py
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
-from . import _ext
+from . import _ext  # type: ignore[attr-defined]
 from dataclasses import dataclass
 from datetime import timedelta
 from gpiod.line import Direction, Bias, Drive, Edge, Clock, Value

I can try to group them by theme, but as this is nothing but internal information, there should be no functional change from this patch (or series of patches). It seems like overkill to submit 8-9 patches.

@brgl
Copy link
Owner

brgl commented Sep 16, 2024

Not by file, by theme ideally. It makes it way easier to review. I don't mind several patches.

@vfazio
Copy link
Contributor Author

vfazio commented Sep 16, 2024

Understood. I'll see what i can do in the next couple of days to split these changes up.

@vfazio
Copy link
Contributor Author

vfazio commented Sep 16, 2024

continuing discussion in #97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants