Add type annotations by bluetech · Pull Request #225 · pytest-dev/pluggy

bluetech · 2019-07-23T19:11:45Z

Based on #224.

This PR adds type annotations to the library, using the Python 2-compatible syntax. The types are not exposed yet (a py.typed file is not distributed).

Motivation: while working on adding type annotations to pytest, I noticed that pluggy is a core part of its machinery and that having type annotations for it will be useful. However, the annotations here turn out to not be that useful, because the main interface, plugin_manager.hook.*, remains untyped (see #191). The API is very dynamic and doesn't lend itself easily to static annotations, though it might be possible, maybe with a mypy plugin. But the types are still useful for all of the regular non-dynamic API. They also help when trying to understand the code base - the type of each variable and how all of the types fit together. Finally, they help catch bugs when the code changes and add confidence when refactoring.

I tested it against pytest (feature branch): first that the test suite passes, and second that the type-checking passes (I manually added a py.typed file to pluggy in the venv to test that).

This PR is probably not that easy to review, adds some maintenance burden, and I also didn't discuss it first. If you don't feel type annotations are desirable for pluggy,, I'll understand :)

`defaults` is the default values e.g. `1` in `foo=1`. It's just used to find the offset of the kwargs, but the code reused the name which made me scratch my head for a minute.

The test passes invalid values to the instance (it does not takes functions, it takes HookImpls). The test fails, but it accidentally used `return` instead of `assert` so it wasn't visible. Since what is being tested is evidently unimportant and legacy, just remove it.

It doesn't take `**kwargs` but an argument `kwargs`.

codecov-io · 2019-07-23T19:27:27Z

Codecov Report

Merging #225 into master will decrease coverage by 4.84%.
The diff coverage is 63.82%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   93.88%   89.04%   -4.85%     
==========================================
  Files           9       14       +5     
  Lines        1129     1826     +697     
  Branches       21      124     +103     
==========================================
+ Hits         1060     1626     +566     
- Misses         66      185     +119     
- Partials        3       15      +12

Impacted Files	Coverage Δ
testing/test_details.py	`91.39% <100%> (+0.09%)`	⬆️
testing/test_invocations.py	`98.61% <100%> (ø)`	⬆️
testing/test_multicall.py	`99.27% <100%> (+0.65%)`	⬆️
testing/test_hookcaller.py	`82.55% <100%> (+0.35%)`	⬆️
src/pluggy/_tracing.py	`80% <31.81%> (ø)`
testing/test_deprecations.py	`91.17% <50%> (ø)`	⬆️
src/pluggy/callers.py	`70.73% <52%> (ø)`
src/pluggy/hooks.py	`83.33% <64.63%> (ø)`
testing/test_helpers.py	`80.7% <66.66%> (-1.12%)`	⬇️
testing/test_tracer.py	`98.61% <66.66%> (-1.39%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aa5592...97a743e. Read the comment docs.

goodboy · 2019-07-24T01:42:01Z

@bluetech definitely something I'm interested in introducing though I don't have time to look through it right now.

I will get to it; feel free to incrementally ping if I take too long.

asottile · 2019-07-24T01:45:55Z

+    from .manager import PluginManager
+
+    _Writer = Callable[[str], None]
+    _Processor = Callable[[Tuple[str, ...], Sequence[object]], None]


should this be Sequence[Any]?

(and throughout)

As you probably know, object and Any both accept any type, but they are opposites in terms of what you can do with them: with object, you can't do much of anything (just __str__ and stuff) without narrowing it first with isinstance, while Any permits anything.

The rule I usually follow is: for inputs that can be anything, I use object - this puts more restrictions on me and provides more assurances to the caller. For outputs, I use Any, since I don't want to force the caller to perform isinstance checks.

(Note: when the use provides a callback to be called by the library, the roles reverse, that is the arguments become Any and the return type becomes object).

I probably got it wrong in a few places so I'll go over it.

asottile · 2019-07-24T01:46:19Z

        self.tags = tags

    def __call__(self, *args):
+        # type: (object) -> None


I think this should be # type: (*Any) -> None

asottile · 2019-07-24T01:48:23Z

+        def __getitem__(self, parameter):  # type: ignore
+            return object
+
+    Generic = _GenericMeta()  # type: ignore


hmm I wonder if we shouldn't just add a dep on typing for py2 and avoid some/most of these (albeit interesting!) fakes

I think these are minor enough that a dependency is not worth it. That said, for me personally py2 is ancient history and the dep would certainly make things cleaner. What do you think?

my vote is the typing;python_version="2.7"

OK, I'll switch to that.

One reason I forgot to mention is that putting everything under if False avoids bringing in the heavy typing import at runtime. But I suppose pytest already "pays" that so it's all the same.

asottile · 2019-07-24T01:49:52Z

        __tracebackhide__ = True
        if self._excinfo is None:
-            return self._result
+            return self._result  # type: ignore


is the # type: ignore because we know it can't be None at this point?

We know it has type _T. However _T itself can be NoneType so an assert is not None is not possible. Will add a comment.

BTW here is some interesting discussion about a nicer Result type with mypy: python/mypy#6936

asottile · 2019-07-24T01:54:04Z


    try:  # func MUST be a function or method here or we won't parse any args
-        spec = _getargspec(func)
+        if hasattr(inspect, "getfullargspec"):


if you have if sys.version_info > (...): instead then the type annotation won't be needed here since mypy will know from typeshed

I'll change that.

asottile · 2019-07-24T01:54:45Z

-        if inspect.ismethod(func) or (
-            "." in getattr(func, "__qualname__", ()) and args[0] in implicit_names
-        ):
+        qualname = getattr(func, "__qualname__", "")  # type: str


hmm why was the annotation necessary here?

Otherwise it'd have type Any.

is that an error? sounds like the settings are cranked to an annoying level then :P

It's not an error, just mypy doesn't know the type so we need to tell it. Can be done without the type comment if we use an hasattr or a try-except but the little comment seemed better.

asottile · 2019-07-24T01:55:48Z

+        return self.metadata["name"]  # type: ignore

-    def __getattr__(self, attr, default=None):
+    def __getattr__(self, attr, default=None):  # type: ignore


what's the # type: ignore here?

warn_return_any: Returning Any from a function declared to return "str".

asottile · 2019-07-24T02:01:00Z

+disallow_any_unimported = True
+disallow_subclassing_any = True
+disallow_untyped_calls = True
+disallow_untyped_defs = True


I find most of the any settings more annoying than they are helpful -- they also tend to lead to lower quality annotations in my experience 🤷‍♂

Even after everything is annotated?

bluetech · 2019-07-26T13:44:21Z

I created two PRs #226 and #227 which will make this PR a bit simpler. I'll wait on them before I update this PR.

bluetech · 2019-09-10T14:09:17Z

This is blocked on some other work, I'll close this now - can be resubmitted later.

jtpavlock · 2020-08-07T00:52:06Z

Is this still blocked? I imagine a lot of the comment annotations could be changed to the normal syntax as indicated in #267

bluetech · 2020-08-08T13:34:08Z

I rebased this branch now: https://github.com/bluetech/pluggy/commits/type-annotations

I think it's good but still needs some self-review.

bluetech added 5 commits July 21, 2019 22:30

Fix error case return type of varnames()

b2ffeb9

Don't confusingly reuse a variable name inside varnames()

92c31e1

`defaults` is the default values e.g. `1` in `foo=1`. It's just used to find the offset of the kwargs, but the code reused the name which made me scratch my head for a minute.

Fix call_historic() example in index.rst

95dd65f

It doesn't take `**kwargs` but an argument `kwargs`.

Add type annotations

97a743e

goodboy requested review from RonnyPfannschmidt, asottile, goodboy and nicoddemus and removed request for asottile and nicoddemus July 24, 2019 01:42

asottile reviewed Jul 24, 2019

View reviewed changes

bluetech closed this Sep 10, 2019

bluetech mentioned this pull request Jan 13, 2020

[WIP] typing improvements pytest-dev/pytest#6449

Closed

bluetech mentioned this pull request Jun 4, 2020

Drop official support for Python 3.4 #267

Closed

bluetech mentioned this pull request Aug 18, 2021

Add typing information to all of pluggy #326

Closed

bluetech mentioned this pull request Jan 10, 2022

Add type annotations #340

Merged

Conversation

bluetech commented Jul 23, 2019

Uh oh!

codecov-io commented Jul 23, 2019

Codecov Report

Uh oh!

goodboy commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluetech commented Jul 26, 2019

Uh oh!

bluetech commented Sep 10, 2019

Uh oh!

jtpavlock commented Aug 7, 2020

Uh oh!

bluetech commented Aug 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants