Add initial mypy support #59

ahawker · 2020-09-08T22:02:46Z

Status: Ready for review
Issue: Fixes #57

If merged, this PR adds initial support for mypy.

Type stubs, .pyi files have been added for the package interface __init__.py and libusb1.py module.
The _version.py module and tests are currently excluded from checks (see mypy.ini). They're skipped now since they're "internal" but can easily be added to type checks in the future.

I did the best I could with the type definitions for functions that I'm not using in https://github.com/adbpy/transports but there are potentially some mistakes/inconsistencies. These should be relatively easily to "whack-o-mole" as they come up (or during code review for someone that knows the codebase better).

Testing

The stubs should pass normal and strict checks.

hawker@mbp:~/src/github.com/ahawker/python-libusb1|master⚡
⇒  mypy usb1 && mypy --strict usb1
Success: no issues found in 4 source files
Success: no issues found in 4 source files

There is also a new Travis CI job for validating the mypy type hints on py36.

I also ran them against all the code in the examples/ directory. All types exported from the package appear correct in there, with the only strict errors being the fact that the examples themselves aren't typed.

The _version.py and testUSB1 modules are both ignored by mypy since they're "internal" to the package. They can be included in the future by simply removing their 'ignore_errors' lines.

PR vpelletier/python-libusb1#59

vpelletier

Which would be the better practice between having the types documenting/type-annotating the code as-is, or documenting/type-annotating only the the non-deprecated, user-relevant parts ?

Some examples of the latter:

USBTransferHelper.__init__ transfer argument could be hidden, along with the submit, cancel and isSubmitted methods
_ReleaseInterface could be an opaque object implementing the context manager protocol

Edit: and I failed at reviewing, I am not done yet but posting this comment submitted it. Oh well...

vpelletier · 2020-09-09T22:49:47Z

usb1/__init__.pyi

+TransferType = int
+TransferStatus = int
+TransferCallback = Callable[['USBTransfer'], None]
+ErrorCallback = Callable[[int], None]


The USBPollerThread.__init__ docstring is inconsistent with the code (...which is itself unnecessary complicated), this should rather be:

ErrorCallback = Callable[[USBError], None]

I fix the docstring and simplify the code.

vpelletier · 2020-09-09T22:58:45Z

usb1/__init__.pyi

+TransferCallback = Callable[['USBTransfer'], None]
+ErrorCallback = Callable[[int], None]
+FileDescriptor = int
+Events = Any


While usb1 itself does not impose restrictions on this argument, it calls it internally with values coming from USBContext.getPollFDList, which come from libusb_pollfd.events, which is a c_short. So used poller need to at least support an integer value (an event mask, as in select.epoll.register & similar).

I believe this boils down to what the mypy best practice is for such case, and you are more familiar with it than I am.

vpelletier · 2020-09-09T23:21:26Z

usb1/__init__.pyi

+UserData = object
+Timeout = int
+PollTimeout = Union[float, int, None]
+TransferType = int


Out of curiosity: is there a way to match these types (TransferType, TransferStatus, ...) with named constants ? So that a check error would happen if these are compared with something else than such named constant, even if it had the same integer value ? (ex: transfer.getType() == LIBUSB_TRANSFER_COMPLETED would fail, even though this test would be accidentally equivalent to ... == LIBUSB_TRANSFER_TYPE_CONTROL).

vpelletier · 2020-09-09T23:36:37Z

usb1/__init__.pyi

+    def isSubmited(self) -> bool: ...
+
+class USBPollerThread(threading.Thread):
+    daemon: bool = ...


I would expect this attribute declaration to be inherited from threading.Thread.

vpelletier · 2020-09-09T23:58:14Z

usb1/__init__.pyi

+                 exc_callback: Optional[ErrorCallback] = ...) -> None: ...
+    def stop(self) -> None: ...
+    @staticmethod
+    def exceptionHandler(exc: BaseException) -> None: ...


Could be specialized a bit more, consistently with ErrorCallback: def exceptionHandler(exc: USBError) -> None: ...

Edit: BTW, I'm surprised that static analysis did not catch this type difference, as __init__ shadows this method with provided exc_callback which (as of this review) has a different type.

vpelletier · 2020-09-10T00:12:24Z

usb1/__init__.pyi

+class USBPoller:
+    def __init__(self,
+                 context: USBContext,
+                 poller: USBPoller) -> None: ...


While USBPoller implements the API it expects from poller, other types are accepted (and in fact they should be the norm). Here I would expect a protocol rather than such stronger typing.

Maybe something like this (not tested):

PollEventMask = int PollResult = List[Tuple[FileDescriptor, PollEventMask]] class PollerType(Protocol): def register(self, fd: FileDescriptor, event_flags: PollEventMask) -> None: ... def unregister(self, fd: FileDescriptor) -> None: ... def poll(timeout: PollTimeout) -> PollResult: ... # ... class USBPoller: def __init__(self, context: USBContext, poller: PollerType) -> None: ...

Edit: And maybe class USBPoller(PollerType): if mypy recognises this as an implementation of that protocol, and lets it fill in the other methods, rather than having to duplicate their declaration (not obvious to me from mypy doc).

vpelletier

One more review checkpoint, I quickly skimmed over the first ~200 lines of libusb.pyi (the integer constants, I did not check that all names are present), and still have the functions to check.

So far, I've only caught a few mistakes, and most of my comments are open-ended: I'm not sure which way is best, and wondering about your opinion.

vpelletier · 2020-09-10T00:59:44Z

usb1/__init__.pyi

+    def getTransfer(self,
+                    iso_packets: int = ...) -> USBTransfer: ...
+
+class USBConfiguration:


__len__ is not specified here. I expected its type was inferred (as it's set to getNumInterfaces), but in further classes you specify it. I'm not sure which one is more correct.

vpelletier · 2020-09-10T01:06:42Z

usb1/__init__.pyi

+
+class USBDevice:
+    device_p: libusb1.libusb_device_p_alias = ...
+    device_descriptor: Any = ...


I do not think I intended these properties as public API (because if one wants to access ctype objects, they should use usb1.libusb directly), so maybe they could be dropped (see also my overall question on what should/should not be typed). If kept, I think this could be changed to libusb1.libusb_device_descriptor type.

vpelletier · 2020-09-10T11:41:51Z

usb1/__init__.pyi

+                                   product_id: Any,
+                                   skip_on_access_error: bool = ...,
+                                   skip_on_error: bool = ...) -> Optional[USBDeviceHandle]: ...
+    def getPollFDList(self) -> Tuple[int, int]: ...


This looks like it should be: def getPollFDList(self) -> List[Tuple[int, int]]: ... (but I did not test)

vpelletier · 2020-09-10T11:46:47Z

usb1/__init__.pyi

+                           added_cb: Optional[FDNotifierCallback] = ...,
+                           removed_cb: Optional[FDNotifierCallback] = ...,
+                           user_data: Optional[UserData] = ...) -> None: ...
+    def getNextTimeout(self) -> int: ...


Strictly, this would produce a float, but I believe mypy is not that strict.

vpelletier · 2020-09-10T11:49:00Z

usb1/__init__.pyi

+    def getNextTimeout(self) -> int: ...
+    def setDebug(self,
+                 level: int) -> None: ...
+    def tryLockEvents(self) -> int: ...


Semantically, this produces a boolean (returns 0 or 1). But type-wise, this is indeed strictly an integer. Not sure which one is best here.

vpelletier · 2020-09-10T11:50:26Z

usb1/__init__.pyi

+    def lockEvents(self) -> None: ...
+    def lockEventWaiters(self) -> None: ...
+    def waitForEvent(self,
+                     tv: int = ...) -> None: ...


Silmilarly to getNextTimeout, tv is actually a float.

vpelletier · 2020-09-10T11:51:27Z

usb1/__init__.pyi

+    def waitForEvent(self,
+                     tv: int = ...) -> None: ...
+    def unlockEventWaiters(self) -> None: ...
+    def eventHandlingOK(self) -> int: ...


Similar to tryLockEvents.

vpelletier · 2020-09-10T11:51:43Z

usb1/__init__.pyi

+    def eventHandlingOK(self) -> int: ...
+    def unlockEvents(self) -> None: ...
+    def handleEventsLocked(self) -> None: ...
+    def eventHandlerActive(self) -> int: ...


Similar to tryLockEvents.

vpelletier · 2020-09-10T11:56:47Z

usb1/__init__.pyi

+CLASS_HID: int
+CLASS_PHYSICAL: int
+CLASS_PTP: int
+CLASS_IMAGE = libusb1


ahawker · 2020-09-14T23:56:57Z

@vpelletier Thank you for taking the the time to do an in-depth review & feedback! Life's gotten a bit busy so it'll be a little slower to tidy this up but just letting you know I haven't forgotten.

vpelletier · 2020-09-15T12:11:19Z

You're welcome. There is no hurry, on my side it's also getting pretty busy, and will probably remain so for about a month.

ahawker added 2 commits September 8, 2020 15:10

Add initial type stubs (.pyi) for package

5b72fdf

Add mypy config

41beb18

The _version.py and testUSB1 modules are both ignored by mypy since they're "internal" to the package. They can be included in the future by simply removing their 'ignore_errors' lines.

ahawker force-pushed the master branch from 6b66e22 to afce856 Compare September 8, 2020 22:11

Add travis ci job for validating mypy type hints

143b563

ahawker force-pushed the master branch from afce856 to 143b563 Compare September 8, 2020 22:17

ahawker added a commit to adbpy/transports that referenced this pull request Sep 9, 2020

Ignore mypy errors in libusb until type hints merged

8da3eca

PR vpelletier/python-libusb1#59

ahawker added a commit to adbpy/transports that referenced this pull request Sep 9, 2020

Ignore mypy errors in libusb until type hints merged

54ceb6e

PR vpelletier/python-libusb1#59

vpelletier reviewed Sep 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial mypy support #59

Add initial mypy support #59

ahawker commented Sep 8, 2020 •

edited

Loading

vpelletier left a comment •

edited

Loading

vpelletier Sep 9, 2020

vpelletier Sep 9, 2020

vpelletier Sep 9, 2020

vpelletier Sep 9, 2020

vpelletier Sep 9, 2020

vpelletier Sep 10, 2020

vpelletier left a comment

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

vpelletier Sep 10, 2020

ahawker commented Sep 14, 2020 •

edited

Loading

vpelletier commented Sep 15, 2020

Add initial mypy support #59

Are you sure you want to change the base?

Add initial mypy support #59

Conversation

ahawker commented Sep 8, 2020 • edited Loading

vpelletier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpelletier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahawker commented Sep 14, 2020 • edited Loading

vpelletier commented Sep 15, 2020

ahawker commented Sep 8, 2020 •

edited

Loading

vpelletier left a comment •

edited

Loading

ahawker commented Sep 14, 2020 •

edited

Loading