mypy: add --disallow-incomplete-defs option #2616

Exirel · 2024-09-01T13:33:10Z

Description

Tin, with the necessary type definition fix. Related to #2603 for Sopel 8.0.1.

Note: I used Any in several places, in particular in sopel.tools.identifiers. I'm not really happy about it. However, the best fix is to properly type annotate instances, i.e. doing something like users: dict[str, User] = SopelIdentifierMemory() as it should properly type check the users sopel memory object.

Checklist

I have read CONTRIBUTING.md
I can and do license this contribution under the EFLv2
No issues are reported by make qa (runs make lint and make test)
I have tested the functionality of the things this change touches

Exirel · 2024-10-12T11:42:25Z

I rebased that over master and it passes the 3.13 mypy check as well!

dgw

It's a constant battle to keep the docs clean, with only the type annotations or hints in the :param: declarations, but not both.

While you go through and look for those (I didn't annotate every one, especially since many are outside where GH will allow line notes), also double check that merging #2285 didn't introduce any new wrinkles for this mypy option? 🙏

sopel/bot.py

Exirel · 2024-10-14T18:49:17Z

but not both.

I fixed the functions I touched, and then fixed some for good measure!

double check that merging #2285 didn't introduce any new wrinkles for this mypy option? 🙏

That would require a rebase first, so you tell me if it's OK and I'll do.

dgw

Lots of good stuff in here, but I do have a few questions (the same one in multiple places, actually) about the choice of type annotation syntax. Overall very impressed with how thorough you were with cleaning up the docstrings!

double check that merging #2285 didn't introduce any new wrinkles for this mypy option? 🙏

That would require a rebase first, so you tell me if it's OK and I'll do.

Now that I think about it, your fixups were tested against the latest master, including the merged changes from #2285. It passed, so you can just squash out the fixups after we settle the syntax question.

sopel/db.py

sopel/plugins/rules.py

sopel/tools/calculation.py

sopel/tools/memories.py

dgw

Cleaned history all good.

I'll have to check other open PRs against this after merging, or make them re-run CI somehow. That's a future-me problem, for later in the week. 😁

SnoopJ

Had some remarks, but if the type checker is happy then so am I!

SnoopJ · 2024-10-14T22:03:17Z

sopel/bot.py

@@ -1030,12 +1034,11 @@ def error(
        self,
        trigger: Optional[Trigger] = None,
        exception: Optional[BaseException] = None,
-    ):
+    ) -> None:
        """Called internally when a plugin causes an error.

        :param trigger: the ``Trigger``\\ing line (if available)


Nit: could probably drop the reference to the type here? I would probably also omit the word "line"

Maybe this would work, but I wouldn't want to take too much context out of the docstring.

Suggested change

:param trigger: the ``Trigger``\\ing line (if available)

:param trigger: the ``Trigger`` that led to the error, if available

sopel/builtins/units.py

SnoopJ · 2024-10-14T22:07:04Z

sopel/builtins/units.py

@@ -199,3 +209,5 @@ def mass(bot, trigger):
        stupid_part = '{:.2f} oz'.format(ounce)

    bot.say('{} = {}'.format(metric_part, stupid_part))
+
+    return None


Same here: I probably would have left off the explicit return

sopel/lifecycle.py

SnoopJ · 2024-10-14T22:10:59Z

sopel/lifecycle.py

@@ -13,21 +13,21 @@
 import inspect
 import logging
 import traceback
-from typing import Callable, Optional
+from typing import Callable


Same here: Is there a reason to prefer the deprecated typing.Callable over collections.abc.Callable?

SnoopJ · 2024-10-14T22:12:20Z

sopel/bot.py

+    Callable,
    Optional,
+    Sequence,


Any reason not to import these from collections.abc since the typing counterparts are deprecated since Python 3.9?

The only reason would be "not touching more than necessary", I think.

Switching from the deprecated imports to their replacements is unrelated to the PR's goal of making --disallow-incomplete-defs work.

SnoopJ · 2024-10-14T22:13:13Z

sopel/plugins/rules.py

@@ -26,6 +26,7 @@
 import threading
 from typing import (
    Any,
+    Callable,


Same comment here about collections.abc vs. typing

SnoopJ · 2024-10-14T22:19:44Z

sopel/tools/calculation.py

+        self,
+        expression_str: str,
+        timeout: float = 5.0,
+    ) -> int | float:


IIRC, int | float is redundant because float can annotate both per PEP 484

I'm okay with explicitly annotating it as a way to document the specific type that will be present but would personally prefer an alias for the union in that case, as a place to leave a comment about why we don't do it the "usual" way.

SnoopJ · 2024-10-14T22:20:48Z

sopel/tools/memories.py

        return super().__contains__(self._make_key(key))

-    def __setitem__(self, key: Optional[str], value):
+    def __setitem__(self, key: str | None, value: Any) -> Any:


Nit: that can be None

Suggested change

def __setitem__(self, key: str | None, value: Any) -> Any:

def __setitem__(self, key: str | None, value: Any) -> None:

Exirel added the Housekeeping Code cleanup, removal of deprecated stuff, etc. label Sep 1, 2024

Exirel added this to the 8.0.1 milestone Sep 1, 2024

Exirel force-pushed the mypy-option-disallow-incomplete-defs branch from b71b791 to ed8f641 Compare October 12, 2024 11:23

Exirel requested a review from a team October 12, 2024 11:42

dgw requested changes Oct 13, 2024

View reviewed changes

sopel/bot.py Show resolved Hide resolved

dgw mentioned this pull request Oct 14, 2024

Pin mypy to 1.11.2 #2631

Merged

4 tasks

dgw reviewed Oct 14, 2024

View reviewed changes

sopel/db.py Show resolved Hide resolved

sopel/plugins/rules.py Show resolved Hide resolved

sopel/tools/calculation.py Show resolved Hide resolved

sopel/tools/memories.py Outdated Show resolved Hide resolved

Exirel added 8 commits October 14, 2024 21:56

tools, calc: fix incomplete type defs

79761ae

irc: fix incomplete type defs

de519b0

lifecycle: fix incomplete type defs

6be67f4

db: fix incomplete type defs

52ad026

bot, plugins: fix incomplete type defs

b786a59

coretasks: fix incomplete type defs

a6cdae5

builtins: fix incomplete type defs

04694df

mypy: add --disallow-incomplete-defs option

dcb9467

Exirel force-pushed the mypy-option-disallow-incomplete-defs branch from bc7b2b5 to dcb9467 Compare October 14, 2024 19:57

dgw approved these changes Oct 14, 2024

View reviewed changes

SnoopJ approved these changes Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mypy: add --disallow-incomplete-defs option #2616

mypy: add --disallow-incomplete-defs option #2616

Exirel commented Sep 1, 2024

Exirel commented Oct 12, 2024

dgw left a comment

Exirel commented Oct 14, 2024

dgw left a comment

dgw left a comment

SnoopJ left a comment

SnoopJ Oct 14, 2024

dgw Oct 15, 2024

SnoopJ Oct 14, 2024

SnoopJ Oct 14, 2024

SnoopJ Oct 14, 2024

dgw Oct 15, 2024

SnoopJ Oct 14, 2024

SnoopJ Oct 14, 2024

SnoopJ Oct 14, 2024

	:param trigger: the ``Trigger``\\ing line (if available)
	:param trigger: the ``Trigger`` that led to the error, if available

	def __setitem__(self, key: str \| None, value: Any) -> Any:
	def __setitem__(self, key: str \| None, value: Any) -> None:

mypy: add --disallow-incomplete-defs option #2616

Are you sure you want to change the base?

mypy: add --disallow-incomplete-defs option #2616

Conversation

Exirel commented Sep 1, 2024

Description

Checklist

Exirel commented Oct 12, 2024

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Oct 14, 2024

dgw left a comment

Choose a reason for hiding this comment

dgw left a comment

Choose a reason for hiding this comment

SnoopJ 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