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

Strict typing and py.typed #18

Merged
merged 8 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

# Be strict about any broken references
nitpicky = True
nitpick_ignore = []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Avasam Avasam Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Include Python intersphinx mapping to prevent failures
# jaraco/skeleton#51
Expand All @@ -40,3 +41,11 @@

# Preserve authored syntax for defaults
autodoc_preserve_defaults = True

# jaraco/pytest-enabler#18
nitpick_ignore += [
('py:class', 'pytest_enabler._T'),
('py:class', '_pytest.config.Config'),
('py:class', '_pytest.config.argparsing.Parser'),
('py:class', 'SupportsRead'),
]
6 changes: 5 additions & 1 deletion mypy.ini
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[mypy]
# Is the project well-typed?
strict = False
strict = True

# Early opt-in even when strict = False
warn_unused_ignores = True
Expand All @@ -12,3 +12,7 @@ explicit_package_bases = True

# Disable overload-overlap due to many false-positives
disable_error_code = overload-overlap

# TODO: Raise issue upstream
[mypy-pytest_cov.*]
ignore_missing_imports = True
1 change: 1 addition & 0 deletions newsfragments/18.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Complete annotations and add ``py.typed`` marker -- by :user:`Avasam`
4 changes: 0 additions & 4 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,3 @@ pytest11 = {enabler = "pytest_enabler"}


[tool.setuptools_scm]


[tool.pytest-enabler.mypy]
# Disabled due to jaraco/skeleton#143
59 changes: 46 additions & 13 deletions pytest_enabler/__init__.py
Original file line number Diff line number Diff line change
@@ -1,27 +1,47 @@
from __future__ import annotations

import contextlib
import os
import pathlib
import re
import shlex
import sys
from collections.abc import Container, MutableSequence, Sequence
from typing import TYPE_CHECKING, TypeVar, overload

import toml
from jaraco.context import suppress
from jaraco.functools import apply
from pytest import Config, Parser

if sys.version_info > (3, 12):
if sys.version_info >= (3, 12):
from importlib import resources
else:
import importlib_resources as resources

if sys.version_info >= (3, 9):
from importlib.abc import Traversable
else:
from pathlib import Path as Traversable

if TYPE_CHECKING:
from _typeshed import SupportsRead
from typing_extensions import Never

import toml
from jaraco.context import suppress
from jaraco.functools import apply

_T = TypeVar("_T")

consume = tuple
consume = tuple # type: ignore[type-arg] # Generic doesn't matter; keep it callable
"""
Consume an iterable
"""


def none_as_empty(ob):
@overload
def none_as_empty(ob: None) -> dict[Never, Never]: ...
@overload
def none_as_empty(ob: _T) -> _T: ...
def none_as_empty(ob: _T | None) -> _T | dict[Never, Never]:
"""
>>> none_as_empty({})
{}
Expand All @@ -33,25 +53,31 @@ def none_as_empty(ob):
return ob or {}


def read_plugins_stream(stream):
def read_plugins_stream(
stream: str | bytes | pathlib.PurePath | SupportsRead[str],
) -> dict[str, dict[str, str]]:
defn = toml.load(stream)
return defn["tool"]["pytest-enabler"]
return defn["tool"]["pytest-enabler"] # type: ignore[no-any-return]


@apply(none_as_empty)
@suppress(Exception)
def read_plugins(path):
def read_plugins(path: Traversable) -> dict[str, dict[str, str]]:
with path.open(encoding='utf-8') as stream:
return read_plugins_stream(stream)


def pytest_load_initial_conftests(early_config, parser, args):
def pytest_load_initial_conftests(
early_config: Config,
parser: Parser | None,
args: MutableSequence[str],
) -> None:
plugins = {
**read_plugins(resources.files().joinpath('default.toml')),
**read_plugins(pathlib.Path('pyproject.toml')),
}

def _has_plugin(name):
def _has_plugin(name: str) -> bool:
pm = early_config.pluginmanager
return pm.has_plugin(name) or pm.has_plugin('pytest_' + name)

Expand All @@ -61,7 +87,7 @@ def _has_plugin(name):
_pytest_cov_check(enabled, early_config, parser, args)


def _remove_deps():
def _remove_deps() -> None:
"""
Coverage will not detect function definitions as being covered
if the functions are defined before coverage is invoked. As
Expand All @@ -82,7 +108,12 @@ def _remove_deps():
consume(map(sys.modules.__delitem__, to_delete))


def _pytest_cov_check(plugins, early_config, parser, args): # pragma: nocover
def _pytest_cov_check(
plugins: Container[str],
early_config: Config,
parser: Parser | None,
args: Sequence[str | os.PathLike[str]],
) -> None: # pragma: nocover
"""
pytest_cov runs its command-line checks so early that no hooks
can intervene. By now, the hook that installs the plugin has
Expand All @@ -99,6 +130,8 @@ def _pytest_cov_check(plugins, early_config, parser, args): # pragma: nocover
_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")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Avasam Avasam Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

parser.parse_known_and_unknown_args(args, early_config.known_args_namespace)

with contextlib.suppress(ImportError):
Expand Down
Empty file added pytest_enabler/py.typed
Empty file.
21 changes: 12 additions & 9 deletions tests/test_enabler.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from __future__ import annotations

import subprocess
import sys
from pathlib import Path
from unittest import mock

import pytest
Expand All @@ -8,52 +11,52 @@


@pytest.fixture
def pyproject(monkeypatch, tmp_path):
def pyproject(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> Path:
monkeypatch.chdir(tmp_path)
return tmp_path / 'pyproject.toml'


def test_pytest_addoption_default():
def test_pytest_addoption_default() -> None:
config = mock.MagicMock()
config.pluginmanager.has_plugin = lambda name: name == 'black'
args = []
args: list[str] = []
enabler.pytest_load_initial_conftests(config, None, args)
assert args == ['--black']


def test_pytest_addoption_override(pyproject):
def test_pytest_addoption_override(pyproject: Path) -> None:
pyproject.write_text(
'[tool.pytest-enabler.black]\naddopts="--black2"\n',
encoding='utf-8',
)
config = mock.MagicMock()
config.pluginmanager.has_plugin = lambda name: name == 'black'
args = []
args: list[str] = []
enabler.pytest_load_initial_conftests(config, None, args)
assert args == ['--black2']


def test_pytest_addoption_disable(pyproject):
def test_pytest_addoption_disable(pyproject: Path) -> None:
pyproject.write_text(
'[tool.pytest-enabler.black]\n#addopts="--black"\n',
encoding='utf-8',
)
config = mock.MagicMock()
config.pluginmanager.has_plugin = lambda name: name == 'black'
args = []
args: list[str] = []
enabler.pytest_load_initial_conftests(config, None, args)
assert args == []


def test_remove_deps(monkeypatch):
def test_remove_deps(monkeypatch: pytest.MonkeyPatch) -> None:
"""
Invoke _remove_deps to push coverage.
"""
monkeypatch.setattr(sys, 'modules', dict(sys.modules))
enabler._remove_deps()


def test_coverage_explicit(tmp_path, monkeypatch):
def test_coverage_explicit(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.chdir(tmp_path)
test = tmp_path.joinpath('test_x.py')
test.write_text('def test_x():\n pass\n', encoding='utf-8')
Expand Down
Loading