Add mypy type checking #908

kurtmckee · 2022-07-09T03:08:50Z

This PR introduces the following changes:

Add mypy as a test environment for tox and CI.
Make watchdog PEP 561 compatible (signaling that it has type annotations).
Remove the emitter_class parameter from BaseObserver.__init__(). This aligns the call signature of the base class with its subclasses and allows the code sample in README.rst to pass mypy checks.

Closes #840

BoboTiG · 2022-07-09T15:42:08Z

There is a breaking change, could you add a line in the changelog for that?

kurtmckee · 2022-07-09T21:33:46Z

Done. Please let me know if this is acceptable, or if other changes are desired. Thanks!

BoboTiG · 2022-07-31T18:17:15Z

Sorry for the delay.

After a second thought, I am not comfortable with the breaking change: it's a historic API, and may be overridden a lot in the nature.
Could you try using another approach that doesn't break the current API?

kurtmckee · 2022-08-01T12:03:39Z

Yep, I'll try to do that. I'll rewrite the history so the commits are clean.

kurtmckee · 2022-08-02T13:15:12Z

I think this is addressed now. I've rebased to keep the commit history clean. Please let me know if additional changes are needed! 👍

tests/test_emitter.py

src/watchdog/observers/__init__.py

altendky · 2023-03-06T21:40:05Z

Is this, or something like it, going to happen? If help is wanted, I'll do what I can. Having a py.typed for a library that isn't well hinted is problematic.

BoboTiG · 2023-03-06T22:09:59Z

@kurtmckee could you fix conflicts 🙏🏻 ?

@altendky I'll review again the PR tomorrow. I would prefer to not remove is_<plateform>() calls in observer/__init__.py. If you know a better way I would love to consider it. Also, it would be great to have another review, if you have some time for that :)

kurtmckee · 2023-03-06T23:10:49Z

Sure thing! It won't be immediate due to work requirements but I can jump back to this soon!

altendky · 2023-03-07T00:15:01Z

I'm skeptical about finding a good solution to avoid platform checks that mypy is aware of. I guess a mypy plugin might be able to use it, but... ick. Could you provide some explanation of what about that code makes it important to keep around? I'm not questioning it yet, but i don't have any of the history to know what makes not changing it important. mypy platform checks have even annoying corner cases as it is.

Thanks to you both for your interest in this both before, and now. :]

altendky · 2023-03-07T00:19:59Z

Hmm, now that I said that...

if (not typing.TYPE_CHECKING and platform.is_linux()) or (typing.TYPE_CHECKING and sys.platform == "linux"):

Maybe? Point being to retain runtime behavior at runtime and preferred type checking behavior when type checking. Still ick, but something.

BoboTiG · 2023-03-07T07:26:56Z

Lets go with the cleanest way that works for mypy. Changes seem good then 👍🏻

altendky · 2023-03-07T15:50:17Z

As a first pass, it looks like all our mypy checks pass with this PR. https://github.com/Chia-Network/chia-blockchain/actions/runs/4355725081 I'll take a quick pass over the changes now and maybe more detailed after ci is green.

kurtmckee · 2023-03-07T16:02:38Z

Thanks, @altendky, that's a confidence boost for the direction of this work! I should be able to return to this soon but have to get through some work requirements first. 😀

altendky

There are some high level things I would encourage doing beyond just this PR. I'm happy to put in my own time towards them and handling them in a separate following PR seems ok.

Run the mypy checks at least across multiple platforms if not also across supported Python versions. Dependencies can vary across each of those. The watchdog code itself certainly varies a lot depending on the platform.
I tend to work with quite strict mypy settings. Applications sort of get a choice but libraries need to live up to whatever standards they want to enable their users to achieve. The default mypy setting mostly only checks stuff that is actively hinted. Requiring that everything is hinted and that Any is banned as much as possible is valuable, along with misc other settings. For the record, I look to https://github.com/altendky/qtrio/blob/459492f5f3d1fe6b6bb8b4e93d9a1bd1cdb4dc32/mypy.ini and https://github.com/Chia-Network/chia-blockchain/blob/8430b072e262cb7a80e688fdc3ad8f1fd2b50d0e/mypy.ini as my personal references for where I've ended up with mypy config.
I try to always ignore the specific error message as opposed to general # type: ignore comments. I see that some ignores here do, many don't. When following this, it is useful to add show_error_codes = True to mypy.ini so they error output includes the error code to ignore.
I certainly don't follow it in all my own cases... but I do like having comments for type ignores. This makes it easier to read and conclude they are appropriate. If possible, a link to a PR or issue that when resolved should result in a reconsideration of the ignore. Sometimes either some or all of the actual mypy complaint is good.

Again, thanks to all for the work here. Based on my mypy checks failing with v2.3.1 but passing with this PR, this is an improvement. Even if you like my suggestions, they can certainly be considered as followups for me to invest my own time in. :]

altendky · 2023-03-07T16:21:44Z

src/watchdog/observers/kqueue.py

+# The `select` module varies between platforms.
+# mypy may complain about missing module attributes
+# depending on which platform it's running on.
+# The comment below disables mypy's attribute check.
+#
+# type: ignore[attr-defined]
+#
+


Big Picture: I think this is a macOS/BSD only option? If so we can encase the whole thing in a platform check instead of ignoring. I guess perhaps a not-windows-not-linux check would be the applicable one here?

Small Picture: Locally, with the just-released mypy 1.1.1 anyways, I get src/watchdog/observers/kqueue.py:23: error: type ignore with error code is not supported for modules; use # mypy: disable-error-code=... [syntax]

altendky · 2023-03-07T16:22:51Z

src/watchdog/observers/winapi.py

@@ -119,11 +119,11 @@ def _errcheck_dword(value, func, args):
    return args


-kernel32 = ctypes.WinDLL("kernel32")
+kernel32 = ctypes.WinDLL("kernel32")  # type: ignore[attr-defined]


With this being Windows only we could again encapsulate the whole file with a platform check. I mean yeah, it's ugly. I take a layer of indentation over lack of good hint checking though, personally.

altendky · 2023-03-07T16:31:38Z

tests/test_emitter.py

@@ -49,7 +52,7 @@
        WindowsApiEmitter as Emitter
    )
 elif platform.is_bsd():
-    from watchdog.observers.kqueue import (
+    from watchdog.observers.kqueue import (  # type: ignore[attr-defined,no-redef]


Do we need to keep around the mypy-incompatible platform checks? Or can we apply the compatible ones everywhere? I think the result in terms of ignoring this code is the same unless we have BSD systems in the test matrix, but just as a general thing I wonder about fully dropping the incompatible check mechanism. Even if there is agreement on that, I understand that that could well end up as a breaking change to save for a major version bump. Though, it does appear to be not documented which makes it at least a little less clearly public API. :] https://watchdog.readthedocs.io/en/latest/api.html#module-watchdog.utils

This allows mypy to perform platform-specific type-checking, which fixes type-checking of the example code in `README.rst`. Closes gorakhargosh#840

kurtmckee · 2023-03-10T14:03:06Z

✔️ Step 1: Rebased on master branch.
✔️ Step 2: Fix new errors that occur with the latest versions of mypy.

I'm going to take a step back from this effort and work on CI, which needs an overhaul. @BoboTiG, expect separate PR's to address some issues in CI.

kurtmckee · 2023-03-10T15:23:43Z

@altendky I'm under the impression that you have a deeper knowledge of type annotations, and how they can be applied in platform-specific ways. Would you be open to @BoboTiG reviewing this PR as-is and then working in parallel toward tightening the type annotations and getting the test suite to pass reliably on all platforms?

altendky · 2023-03-10T15:37:16Z

Yep, works for me. As is this is useful for both of us.

BoboTiG · 2023-03-10T15:44:36Z

Thanks both of you. It will be useful to a lot more people at the end :)

kurtmckee force-pushed the add-mypy-type-checking branch from c590714 to f535a8c Compare August 2, 2022 13:14

BoboTiG reviewed Aug 2, 2022

View reviewed changes

tests/test_emitter.py Outdated Show resolved Hide resolved

src/watchdog/observers/__init__.py Show resolved Hide resolved

kurtmckee force-pushed the add-mypy-type-checking branch 2 times, most recently from ef1fdeb to d03274d Compare August 4, 2022 13:18

altendky mentioned this pull request Mar 6, 2023

Bump watchdog from 2.2.0 to 2.3.1 Chia-Network/chia-blockchain#14709

Closed

altendky mentioned this pull request Mar 7, 2023

update watchdog Chia-Network/chia-blockchain#14768

Draft

2 tasks

altendky reviewed Mar 7, 2023

View reviewed changes

kurtmckee added 5 commits March 10, 2023 07:54

Add mypy as a tox test environment

8eed541

Fix all of the issues reported by mypy

07116f3

Update platform-specific Observer imports so mypy understands them

0fe2700

This allows mypy to perform platform-specific type-checking, which fixes type-checking of the example code in `README.rst`. Closes gorakhargosh#840

Add a changelog entry

1b9dfcf

Add mypy as a CI environment

81d49b2

kurtmckee force-pushed the add-mypy-type-checking branch from d03274d to 81d49b2 Compare March 10, 2023 14:00

Resolve an error with mypy 1.1.1

8e46f24

BoboTiG merged commit 4d1510d into gorakhargosh:master Mar 10, 2023

kurtmckee deleted the add-mypy-type-checking branch March 10, 2023 16:18

BoboTiG mentioned this pull request Mar 12, 2023

Type of Observer is unknown #942

Closed

Uh oh!

Add mypy type checking #908

Add mypy type checking #908

Uh oh!

Conversation

kurtmckee commented Jul 9, 2022

Uh oh!

BoboTiG commented Jul 9, 2022

Uh oh!

kurtmckee commented Jul 9, 2022

Uh oh!

BoboTiG commented Jul 31, 2022

Uh oh!

kurtmckee commented Aug 1, 2022

Uh oh!

kurtmckee commented Aug 2, 2022

Uh oh!

Uh oh!

Uh oh!

altendky commented Mar 6, 2023

Uh oh!

BoboTiG commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtmckee commented Mar 6, 2023

Uh oh!

altendky commented Mar 7, 2023

Uh oh!

altendky commented Mar 7, 2023

Uh oh!

BoboTiG commented Mar 7, 2023

Uh oh!

altendky commented Mar 7, 2023

Uh oh!

kurtmckee commented Mar 7, 2023

Uh oh!

altendky left a comment

Choose a reason for hiding this comment

Uh oh!

altendky Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

altendky Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

altendky Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

kurtmckee commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtmckee commented Mar 10, 2023

Uh oh!

altendky commented Mar 10, 2023

Uh oh!

BoboTiG commented Mar 10, 2023

Uh oh!

Uh oh!

BoboTiG commented Mar 6, 2023 •

edited

Loading

kurtmckee commented Mar 10, 2023 •

edited

Loading