test: implement type hints

[kuwv]

[EDITOR'S NOTE TO FUTURE SELF: I specifically requested that @kuwv poke at this so I could have a gander at what the codebase feels like with typing rubbed on it. See #904.]

[Dreamsorcerer]

@bitprophet If you're happy to add typing support, I'm happy to help with reviews. I've done a lot typing across the aio-libs projects and a few others.

Note that typing can be done even more gradually than this PR by using the config file (e.g. you could essentially add a blacklist of files/directories that should ignore errors, then gradually remove them from the blacklist 1 PR at a time).

[kuwv]

@bitprophet, @Dreamsorcerer the merge is near complete. The typing_extensions is 4.1.1 for compatibility with 3.6 but probably need to limit import to 3.6 and 3.7. Coverage percentage is off though.

There's a few fixes in code but some are commented instead:

./invoke/executor.py:                # FIXME: task.name can be none here
./invoke/parser/parser.py:            initial=self.initial,  # type: ignore # FIXME: should not be none
./invoke/loader.py:            # FIXME: deprecated capability that needs replacement
./invoke/completion/complete.py:        # FIXME: this seems wonky
./invoke/context.py:        # FIXME pattern should be raw string prompt.encode('unicode_escape')

If you or anyone else would like to see it:

python -m venv .venv
source .venv/bin/activate
pip install -r dev-requirements.txt
mypy .

Half of the benefits of using mypy is documenting the API but it requires setup with your IDE (or vim/ale).

[Dreamsorcerer]

@bitprophet Let me know if you're happy to go with something like this, and then I'll pull it locally and do a full review.

[kuwv]

@bitprophet could you let us know what your thoughts are? I'd like to have @Dreamsorcerer feedback while we got him. Thanks!

[bitprophet]

Taking a look now, though my general feeling so far is positive 👍🏻

[Dreamsorcerer]

I've also added TODO comments for typing enhancements that can be added in a later Python version, or with typing_extensions (I'll leave it to you to decide whether you want to bring typing_extensions in again or not, seems we don't need it currently).

Yeah, I'll probably add it again later when I rewrite #698.

Sure, but as before, I'd recommend not vendoring it if you bring it in. mypy special cases typing_extensions, and by pulling it into the repo, you lose that special case and require mypy to parse the whole thing, which seems to result in additional errors.

Do you want to add typing to tests? It seems that there are some issues with it if we do.

The main thing is just figuring out what is going on with those classes and whether it's worth simplifying them to something more intuitive. All the tests seem to be written like this:

class Foo:
    def foo(self): ...

    class InnerFoo:
        def test(self):
            self.foo()  # How can this be defined?

In normal Python code, that is clearly broken. If those tests are actually working, then pytest or something must be doing something weird to modify the classes.

[Dreamsorcerer]

An obvious solution would be to use normal inheritance:

class Foo:
    def foo(self): ...

class InnerFoo(Foo):
    def test(self):
        self.foo()

[kuwv]

An obvious solution would be to use normal inheritance:

class Foo:

    def foo(self): ...



class InnerFoo(Foo):

    def test(self):

        self.foo()

I'd like to wait on doing the tests since the tasks and argument need more attention.

Essentially, 'task' is a decorator wrapping another decorator. This is causing Callable => Task => Callable.

[Dreamsorcerer]

I'd like to wait on doing the tests since the tasks and argument need more attention.

That was my suggestion. It'd be too much to change tests in this PR.

Essentially, 'task' is a decorator wrapping another decorator. This is causing Callable => Task => Callable.

Think this is already done in my suggested changes?

[Dreamsorcerer]

Let me know if you want me to put those changes into a PR, so they can reviewed separately.

[bitprophet]

Hey @Dreamsorcerer, @kuwv - I'll try to review the rest of this, and the discussion, today. At a high level, +1 on waiting on tests (it's an idiosyncratic pytest plugin that I'm not currently willing to drop, so if it means they cannot be reasonably typechecked, then maybe that's just moot).

---

What exactly is the deal with typing_extensions? Is it something we can specify as part of the development-only dependency list (and perhaps add to some docs in case that's insufficient for folks relying on our type annotations incidentally)?

I like the idea of supporting typing, but not to the point where we'd grow our first non-vendored dependency solely to support typechecking. (Maybe as a setuptools extra, eg invoke[typed/typing/whatever]? That might be the best of both worlds?)

[kuwv]

Let me know if you want me to put those changes into a PR, so they can reviewed separately.

I've updated most of your suggestions. I'm looking at the 'task' library though. It's not using 'decorator' lib or 'wraps'. It needs to provide its metadata and the wrapped function but it's not doing it idiomatically.

I'll be traveling and I'll be out of pocket here and there.

[Dreamsorcerer]

I'm looking at the 'task' library though. It's not using 'decorator' lib or 'wraps'. It needs to provide its metadata and the wrapped function but it's not doing it idiomatically.

I probably wasn't looking too closely, it just looked like something that wraps a function (similar to asyncio.Task), so I didn't think about wraps(). If that's relevant, feel free to add it in, but not sure it's needed for typing anyway.

[Dreamsorcerer]

What exactly is the deal with typing_extensions?

Pretty much includes newer typing features for backwards compatibility (including some experimental features not yet in a Python release).

Typical use case is to do something like:

if sys.version_info >= (3, 8):
    from typing import Literal
else:
    from typing_extensions import Literal

Then you can add it as a dependency only for older Python releases, for example in aiohttp we only depend on it for Python 3.7:
https://github.com/aio-libs/aiohttp/blob/3.9/setup.cfg#L56

As I said above, if you don't want to depend on it at all, I've put TODOs in that can be updated when you drop support for the preceding Python version. The TODOs improve the accuracy of typing, but they are not game-changing.

Is it something we can specify as part of the development-only dependency list (and perhaps add to some docs in case that's insufficient for folks relying on our type annotations incidentally)?

No, it is needed at runtime.

[Dreamsorcerer]

As I said above, if you don't want to depend on it at all, I've put TODOs in that can be updated when you drop support for the preceding Python version.

When I drop support for a Python version, I actually look through the code base with (e.g. when dropping Python 3.6):

fgrep -Rn -e '36' -e '3.6' -e '3,6' -e '3, 6'
fgrep -Rn -e '37' -e '3.7' -e '3,7' -e '3, 7'

Which helps me to find all the TODOs and version_info checks that can now be removed.

[bitprophet]

Yea, I tend to do similar, # TODO: nuke/change/etc when dropping Python 3.6, then ack -i "py(thon)? ?3\.6" or whatnot.

And more to the point, acknowledged, thanks for the explainer. Given this is about accuracy/gentle degradation of typing ability, and not "its lack makes typechecking impossible or completely pointless", I think I'm fine with the TODOS+eventual updating approach.

I'm also open to changing my approach if I really get the typing religion and/or we get a lot of folks using this initial release of support who then return with "argh the lack of what typing_extensions would enable, is really killing us here!".

[bitprophet]

FWIW given both of y'all have already put a lot of time into this and one of you, at least, is partly AFK - I'm likely to make some executive decisions and kick some version of this out the door without a ton more back/forth, so the community can try it out.

Especially since my read of annotations is that we can hone them over time without it being a huge headache for people (vs "real" APIs which can break folks pretty bad if they change unexpectedly). So both gradual extensions of the annotations & occasional tweaking seem doable w/o a lot of hand-wringing. (I do so love hand-wringing...)

[bitprophet]

Finished reviewing Jesse's changes, made a small pile of my own where I disagreed, and now rolling through Sam's suggestions-via-commit (which I expect to manually apply so it doesn't pull in the test suite changes). Thanks again for all of this!

[bitprophet]

Whoops, I see @kuwv doing some of the same thing this evening (pulling in Sam's changes). I'll stop what I'm doing and wait for a go-ahead 😂 I have a handful of my own changes - see https://github.com/pyinvoke/invoke/tree/906-int if interested. I'll have to rebase/cherrypick them on top of whatever is added in here.

I will note that so far I've liked Sam's changes except for the Generics stuff around the Task class and task decorator; which I continue to find pretty confusing. While the more (heh) generic uses of Any+Callable that Jesse started out with is, surely, a lot more broad than the use of the overload decorator + TypeVar/Generic, I find it much easier to read/grok offhand.

[Dreamsorcerer]

I will note that so far I've liked Sam's changes except for the Generics stuff in invoke.tasks, which I continue to find pretty confusing. While the more (heh) generic use of Any that Jesse started out with is, surely, a lot more broad than the use of the overload decorator + TypeVar/Generic, I find it much easier to read/grok offhand.

Let me know if you want to go over anything. Any just means dynamic typing, so we lose all typing benefits whenever you see that. So, any function passed into a task becomes untyped, which for such a generic object is rather undesirable.

The TypeVar is almost like a placeholder and just means you'll get the same type out as you put in (in this case the _T in the body defines the return type that comes from __call__()). So, with the TypeVar in place this will work:

def foo() -> str: ...

t = Task(foo)
result = t()
reveal_type(result)  # str

Without that, you'll get Any.

The overloads are just for the decorator, which is a little awkward because it can be used as @task or @task(), so it's just trying to define both cases. Admittedly, the arguments in the signature feel slightly mangled in order to make it work, but again if you don't do that then every single function you put that decorator on is going to become untyped.

[Dreamsorcerer]

I think the only thing I'm not entirely certain on is invoke.parser.argument.Argument, which is really rather complicated from a static typing perspective, and I'm not sure how precise the types are. Because it has things like value and raw_value, it can have a default value, it can also be None, it might be a list which then append values, and set_value() may cast or not cast; it's all rather complicated as to what the correct types might be...

[kuwv]

@bitprophet I decided I needed to provide my latest updates. Sam fixed a bunch of stuff I overlooked. I'll leave the rest for you to decide. Still need to tune argument and tasks though but that can wait.

[bitprophet]

invoke.parser.argument.Argument, which is really rather complicated from a static typing perspective

Arguments literally stand in for CLI flags - so like most other such libraries, the possibilities for what type they end up presenting as (and their default values, etc) are indeed extremely varied. I'm not sure offhand what the "right" way to do this would be (ie if one were to write Invoke from scratch with typing in mind). Probably a tree of subclasses that could each hold more specific "once you get a value out" type hints? ListArg, etc.

I'm happy to kick the can down the road a bit for the thornier corners here - I assume that just being able to run mypy on client codebases (without having to mark Invoke as entirely untyped, as they would have prior) will be a big step up for many folks, and as I noted upthread, I expect we'll streamline this in the future, both the bits that are already happily typed here, and the bits that are in question or Any'd.

[kuwv]

@bitprophet I think this might have gotten reverted.

[bitprophet]

Yea, that's messed up. Clearly lazygit is a little too powerful sometimes. Will try to get it back.

[bitprophet]

@kuwv Should be restored now. Suspect this was due to me trying to juggle changes between 2.0 and main, must have somehow reset main to be equal to a version of 2.0. The trouble with trying to keep even a single stable branch around for folks…