Strict typing and py.typed

[Avasam]

Ref: jaraco/skeleton#143

[jaraco]

Very nice. Thanks for doing it this way. I'll add this line to the skeleton as well for simpler diffs.

[Avasam]

If you add it to skeleton, you might want to write in a way where https://mypy.readthedocs.io/en/stable/error_code_list.html#require-annotation-if-variable-type-is-unclear-var-annotated won't trigger when it isn't being re-assigned and the type cannot be inferred.

from __future__ import annotations

...

nitpick_ignore: list[tuple[str, str]] = []

or

# mypy: disable-error-code="var-annotated"

...

nitpick_ignore = []

(or disable var-annotated in mypy.ini for docs/conf.py)

[jaraco]

Oh! Nice recommendation. Good thing our instincts align. I added jaraco/skeleton@0c326f3 before I saw this. I hope I got the type right. I'm testing it now.

[jaraco]

Looks great. Nice work. Just one last concern and it's good to go.

[jaraco]

This new code block injects itself between the comment and the comment's logic.

I'd like to avoid adding unnecessary logic and unreachable code to satisfy the type checker. I'd rather see a cast, probably at the call to _pytest_cov_check so that parser can be typed as parser: Parser (even though technically, parser could be None). Another way might be to change the code above to:

    if 'cov' not in plugins or parser is None:

Since it's a goal not to change the logic when adding type declarations (except where legitimate bugs are detected), let's go with the cast to the caller(s) and change the function to expect parser: Parser.

[Avasam]

if 'cov' not in plugins or parser is None: at line 123 works, but that one changes the logic: before if you had early_config.pluginmanager.getplugin('_cov') returning True, you'd be allowed to have a None parser. And _remove_deps() would no longer be called.

The current change only changes the error message and type (from AttributeError to ValueError)
Up to you how a None parser that reaches this code should be handled.

[Avasam]

A # type: ignore[union-attr] # TODO: Handle None parser works as well for me, as long as it's clear why the suppression comment is there.

[jaraco]

if 'cov' not in plugins or parser is None: at line 123 works, but that one changes the logic: before if you had early_config.pluginmanager.getplugin('_cov') returning True, you'd be allowed to have a None parser. And _remove_deps() would no longer be called.

Good catch. I was working from the assumption that the message in the ValueError("parser cannot be None if cov in plugins") would hold, but it sounds like it's more complicated than that.

Here's what I have in mind:

diff --git a/pytest_enabler/__init__.py b/pytest_enabler/__init__.py
index bc8f10f..010a214 100644
--- a/pytest_enabler/__init__.py
+++ b/pytest_enabler/__init__.py
@@ -7,7 +7,7 @@ import re
 import shlex
 import sys
 from collections.abc import Container, MutableSequence, Sequence
-from typing import TYPE_CHECKING, TypeVar, overload
+from typing import TYPE_CHECKING, TypeVar, cast, overload
 
 import toml
 from jaraco.context import suppress
@@ -84,7 +84,14 @@ def pytest_load_initial_conftests(
     enabled = {key: plugins[key] for key in plugins if _has_plugin(key)}
     for plugin in enabled.values():
         args.extend(shlex.split(plugin.get('addopts', "")))
-    _pytest_cov_check(enabled, early_config, parser, args)
+    _pytest_cov_check(
+        enabled,
+        early_config,
+        # parser is only used when known not to be None
+        # based on `enabled` and `early_config`.
+        cast(Parser, parser),
+        args,
+    )
 
 
 def _remove_deps() -> None:
@@ -111,7 +118,7 @@ def _remove_deps() -> None:
 def _pytest_cov_check(
     plugins: Container[str],
     early_config: Config,
-    parser: Parser | None,
+    parser: Parser,
     args: Sequence[str | os.PathLike[str]],
 ) -> None:  # pragma: nocover
     """
@@ -130,8 +137,6 @@ def _pytest_cov_check(
     _remove_deps()
     # important: parse all known args to ensure pytest-cov can configure
     # itself based on other plugins like pytest-xdist (see #1).
-    if parser is None:
-        raise ValueError("parser cannot be None if cov in plugins")
     parser.parse_known_and_unknown_args(args, early_config.known_args_namespace)
 
     with contextlib.suppress(ImportError):

(8657ea1)

I realize it's a little convoluted to have the caller do the cast, but I like that the type system is protecting against the unwanted type. What I'd really like, if it weren't so messy, is a wrapper for _pytest_cov_check that handles the first half (validating the inputs or short circuiting) and a second half (invoking the behavior against known inputs), something like b8d7efd.

A # type: ignore[union-attr] # TODO: Handle None parser works as well for me, as long as it's clear why the suppression comment is there.

I'd also be okay with this approach. I'll let you decide which of the three options you like best.